refactor: do not infer project root from script location (#1265)

* fix(Api): do not infer project root from script location * fix(builders): do not infer project root from script location * fix(target): do not infer project root from script location * test(e2e): cleanup and extend E2E tests - Renames the file with the only existing E2E test - Makes existing test use the API instance returned by `Api.createPlatform` - Adds another test that ensures we can still require the API from `platformProjectPath/cordova/Api.js` * fix(check_reqs): do not infer project root from script location
apache · Jul 13, 2021 · 1f0ea17 · 1f0ea17
1 parent 70a1eff
commit 1f0ea17
Show file tree

Hide file tree

Showing 14 changed files with 142 additions and 124 deletions.
diff --git a/bin/lib/create.js b/bin/lib/create.js
@@ -230,7 +230,7 @@ exports.create = function (project_path, config, options, events) {
         ? config.name().replace(/[^\w.]/g, '_') : 'CordovaExample';
 
     var safe_activity_name = config.android_activityName() || options.activityName || 'MainActivity';
-    var target_api = check_reqs.get_target();
+    var target_api = check_reqs.get_target(project_path);
 
     // Make the package conform to Java package types
     return exports.validatePackageName(package_name)

diff --git a/bin/templates/cordova/Api.js b/bin/templates/cordova/Api.js
@@ -66,7 +66,7 @@ function setupEvents (externalEventEmitter) {
 class Api {
     constructor (platform, platformRootDir, events) {
         this.platform = PLATFORM;
-        this.root = path.resolve(__dirname, '..');
+        this.root = platformRootDir;
 
         setupEvents(events);
 
@@ -313,7 +313,7 @@ class Api {
      *   objects for current platform.
      */
     requirements () {
-        return require('./lib/check_reqs').check_all();
+        return require('./lib/check_reqs').check_all(this.root);
     }
 
     /**

diff --git a/bin/templates/cordova/lib/builders/ProjectBuilder.js b/bin/templates/cordova/lib/builders/ProjectBuilder.js
@@ -78,7 +78,7 @@ function findOutputFiles (bundleType, buildType, { arch } = {}) {
 
 class ProjectBuilder {
     constructor (rootDirectory) {
-        this.root = rootDirectory || path.resolve(__dirname, '../../..');
+        this.root = rootDirectory;
         this.apkDir = path.join(this.root, 'app', 'build', 'outputs', 'apk');
         this.aabDir = path.join(this.root, 'app', 'build', 'outputs', 'bundle');
     }
@@ -310,7 +310,7 @@ class ProjectBuilder {
             if (error.toString().includes('failed to find target with hash string')) {
                 // Add hint from check_android_target to error message
                 try {
-                    await check_reqs.check_android_target();
+                    await check_reqs.check_android_target(this.root);
                 } catch (checkAndroidTargetError) {
                     error.message += '\n' + checkAndroidTargetError.message;
                 }

diff --git a/bin/templates/cordova/lib/check_reqs.js b/bin/templates/cordova/lib/check_reqs.js
@@ -22,7 +22,6 @@ var path = require('path');
 var fs = require('fs-extra');
 const { forgivingWhichSync, isWindows, isDarwin } = require('./utils');
 const java = require('./env/java');
-var REPO_ROOT = path.join(__dirname, '..', '..', '..', '..');
 const { CordovaError, ConfigParser, events } = require('cordova-common');
 var android_sdk = require('./android_sdk');
 const { SDK_VERSION } = require('./gradle-config-defaults');
@@ -32,10 +31,11 @@ const { SDK_VERSION } = require('./gradle-config-defaults');
 Object.assign(module.exports, { isWindows, isDarwin });
 
 /**
+ * @param {string} projectRoot
  * @returns {string} The android target in format "android-${target}"
  */
-module.exports.get_target = function () {
-    const userTargetSdkVersion = getUserTargetSdkVersion();
+module.exports.get_target = function (projectRoot) {
+    const userTargetSdkVersion = getUserTargetSdkVersion(projectRoot);
 
     if (userTargetSdkVersion && userTargetSdkVersion < SDK_VERSION) {
         events.emit('warn', `android-targetSdkVersion should be greater than or equal to ${SDK_VERSION}.`);
@@ -44,10 +44,16 @@ module.exports.get_target = function () {
     return `android-${Math.max(userTargetSdkVersion, SDK_VERSION)}`;
 };
 
-/** @returns {number} target sdk or 0 if undefined */
-function getUserTargetSdkVersion () {
+/**
+ * @param {string} projectRoot
+ * @returns {number} target sdk or 0 if undefined
+ */
+function getUserTargetSdkVersion (projectRoot) {
     // If the repo config.xml file exists, find the desired targetSdkVersion.
-    const configFile = path.join(REPO_ROOT, 'config.xml');
+    // We need to use the cordova project's config.xml here, since the platform
+    // project's config.xml does not yet have the user's preferences when this
+    // function is called during `Api.createPlatform`.
+    const configFile = path.join(projectRoot, '../../config.xml');
     if (!fs.existsSync(configFile)) return 0;
 
     const configParser = new ConfigParser(configFile);
@@ -235,13 +241,13 @@ module.exports.check_android = function () {
     });
 };
 
-module.exports.check_android_target = function () {
+module.exports.check_android_target = function (projectRoot) {
     // valid_target can look like:
     //   android-19
     //   android-L
     //   Google Inc.:Google APIs:20
     //   Google Inc.:Glass Development Kit Preview:20
-    var desired_api_level = module.exports.get_target();
+    var desired_api_level = module.exports.get_target(projectRoot);
     return android_sdk.list_targets().then(function (targets) {
         if (targets.indexOf(desired_api_level) >= 0) {
             return targets;
@@ -286,9 +292,10 @@ var Requirement = function (id, name, version, installed) {
  * Methods that runs all checks one by one and returns a result of checks
  * as an array of Requirement objects. This method intended to be used by cordova-lib check_reqs method
  *
+ * @param {string} projectRoot
  * @return Promise<Requirement[]> Array of requirements. Due to implementation, promise is always fulfilled.
  */
-module.exports.check_all = function () {
+module.exports.check_all = function (projectRoot) {
     var requirements = [
         new Requirement('java', 'Java JDK'),
         new Requirement('androidSdk', 'Android SDK'),
@@ -299,7 +306,7 @@ module.exports.check_all = function () {
     var checkFns = [
         this.check_java,
         this.check_android,
-        this.check_android_target,
+        this.check_android_target.bind(this, projectRoot),
         this.check_gradle
     ];
 

diff --git a/bin/templates/cordova/lib/run.js b/bin/templates/cordova/lib/run.js
@@ -21,6 +21,7 @@ var emulator = require('./emulator');
 const target = require('./target');
 const build = require('./build');
 const PackageType = require('./PackageType');
+const AndroidManifest = require('./AndroidManifest');
 const { CordovaError, events } = require('cordova-common');
 
 /**
@@ -75,5 +76,8 @@ module.exports.run = async function (runOptions = {}) {
     if (resolvedTarget.type === 'emulator') {
         await emulator.wait_for_boot(resolvedTarget.id);
     }
-    return target.install(resolvedTarget, buildResults);
+
+    const manifest = new AndroidManifest(this.locations.manifest);
+
+    return target.install(resolvedTarget, { manifest, buildResults });
 };
diff --git a/bin/templates/cordova/lib/target.js b/bin/templates/cordova/lib/target.js
@@ -17,13 +17,11 @@
        under the License.
 */
 
-const path = require('path');
 const { inspect } = require('util');
 const execa = require('execa');
 const Adb = require('./Adb');
 const build = require('./build');
 const emulator = require('./emulator');
-const AndroidManifest = require('./AndroidManifest');
 const { compareBy } = require('./utils');
 const { retryPromise } = require('./retry');
 const { events, CordovaError } = require('cordova-common');
@@ -129,9 +127,8 @@ exports.resolve = async (spec, buildResults) => {
     };
 };
 
-exports.install = async function ({ id: target, arch, type }, buildResults) {
+exports.install = async function ({ id: target, arch, type }, { manifest, buildResults }) {
     const apk_path = build.findBestApkForArchitecture(buildResults, arch);
-    const manifest = new AndroidManifest(path.join(__dirname, '../../app/src/main/AndroidManifest.xml'));
     const pkgName = manifest.getPackageId();
     const launchName = pkgName + '/.' + manifest.getActivity().getName();
 

diff --git a/spec/e2e/e2e.spec.js b/spec/e2e/e2e.spec.js
@@ -0,0 +1,74 @@
+/*
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements.  See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership.  The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License.  You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied.  See the License for the
+ specific language governing permissions and limitations
+ under the License.
+ */
+
+const os = require('os');
+const fs = require('fs-extra');
+const path = require('path');
+const { EventEmitter } = require('events');
+const { ConfigParser, PluginInfoProvider } = require('cordova-common');
+const Api = require('../../bin/templates/cordova/Api');
+
+function makeTempDir () {
+    const tmpDirTemplate = path.join(os.tmpdir(), 'cordova-android-test-');
+    return fs.realpathSync(fs.mkdtempSync(tmpDirTemplate));
+}
+
+async function makeProject (projectPath) {
+    const configXmlPath = path.join(__dirname, '../../bin/templates/project/res/xml/config.xml');
+    const config = new ConfigParser(configXmlPath);
+    config.setPackageName('io.cordova.testapp');
+    config.setName('TestApp');
+
+    const noopEvents = new EventEmitter();
+
+    return Api.createPlatform(projectPath, config, {}, noopEvents);
+}
+
+describe('E2E', function () {
+    let tmpDir, projectPath, api;
+    beforeEach(async () => {
+        tmpDir = makeTempDir();
+
+        projectPath = path.join(tmpDir, 'project');
+        api = await makeProject(projectPath);
+    });
+    afterEach(() => {
+        fs.removeSync(tmpDir);
+    });
+
+    it('loads the API from a project directory', async () => {
+        // Allow test project to find the `cordova-android` module
+        fs.ensureSymlinkSync(
+            path.join(__dirname, '../..'),
+            path.join(tmpDir, 'node_modules/cordova-android'),
+            'junction'
+        );
+
+        expect(() => {
+            require(path.join(projectPath, 'cordova/Api.js'));
+        }).not.toThrow();
+    });
+
+    it('adds a plugin with framework', async () => {
+        const fakePluginPath = path.join(__dirname, 'fixtures/cordova-plugin-fake');
+        const pluginInfo = new PluginInfoProvider().get(fakePluginPath);
+
+        await expectAsync(api.addPlugin(pluginInfo)).toBeResolved();
+    });
+});
diff --git a/spec/e2e/plugin.spec.js b/spec/e2e/plugin.spec.js
diff --git a/spec/unit/builders/ProjectBuilder.spec.js b/spec/unit/builders/ProjectBuilder.spec.js
@@ -43,10 +43,8 @@ describe('ProjectBuilder', () => {
             expect(builder.root).toBe(rootDir);
         });
 
-        it('should set the project directory to three folders up', () => {
-            ProjectBuilder.__set__('__dirname', 'projecttest/platforms/android/app');
-            builder = new ProjectBuilder();
-            expect(builder.root).toMatch(/projecttest$/);
+        it('should throw if project directory is missing', () => {
+            expect(() => new ProjectBuilder()).toThrowError(TypeError);
         });
     });
 
@@ -224,7 +222,7 @@ describe('ProjectBuilder', () => {
             return builder.build({}).then(
                 () => fail('Unexpectedly resolved'),
                 error => {
-                    expect(checkReqsSpy.check_android_target).toHaveBeenCalled();
+                    expect(checkReqsSpy.check_android_target).toHaveBeenCalledWith(rootDir);
                     expect(error).toBe(testError);
                 }
             );

diff --git a/spec/unit/builders/builders.spec.js b/spec/unit/builders/builders.spec.js
@@ -31,8 +31,10 @@ describe('builders', () => {
 
     describe('getBuilder', () => {
         it('should return an instance of ProjectBuilder when gradle is requested', () => {
-            const newBuilder = builders.getBuilder();
+            const root = 'FakeProjectRoot';
+            const newBuilder = builders.getBuilder(root);
             expect(newBuilder).toEqual(jasmine.any(ProjectBuilder));
+            expect(newBuilder.root).toBe(root);
         });
 
         it('should throw an error if a builder cannot be instantiated', () => {

diff --git a/spec/unit/check_reqs.spec.js b/spec/unit/check_reqs.spec.js
@@ -262,6 +262,7 @@ describe('check_reqs', function () {
     });
 
     describe('get_target', function () {
+        const projectRoot = 'fakeProjectRoot';
         var ConfigParser;
         var getPreferenceSpy;
         beforeEach(function () {
@@ -273,7 +274,7 @@ describe('check_reqs', function () {
         });
 
         it('should retrieve DEFAULT_TARGET_API', function () {
-            var target = check_reqs.get_target();
+            var target = check_reqs.get_target(projectRoot);
             expect(target).toBeDefined();
             expect(target).toContain('android-' + DEFAULT_TARGET_API);
         });
@@ -282,7 +283,7 @@ describe('check_reqs', function () {
             spyOn(fs, 'existsSync').and.returnValue(true);
             getPreferenceSpy.and.returnValue(String(DEFAULT_TARGET_API + 1));
 
-            var target = check_reqs.get_target();
+            var target = check_reqs.get_target(projectRoot);
 
             expect(getPreferenceSpy).toHaveBeenCalledWith('android-targetSdkVersion', 'android');
             expect(target).toBe('android-' + (DEFAULT_TARGET_API + 1));
@@ -292,7 +293,7 @@ describe('check_reqs', function () {
             spyOn(fs, 'existsSync').and.returnValue(true);
             getPreferenceSpy.and.returnValue('android-99');
 
-            var target = check_reqs.get_target();
+            var target = check_reqs.get_target(projectRoot);
 
             expect(getPreferenceSpy).toHaveBeenCalledWith('android-targetSdkVersion', 'android');
             expect(target).toBe('android-' + DEFAULT_TARGET_API);
@@ -305,7 +306,7 @@ describe('check_reqs', function () {
 
             getPreferenceSpy.and.returnValue(String(DEFAULT_TARGET_API - 1));
 
-            var target = check_reqs.get_target();
+            var target = check_reqs.get_target(projectRoot);
 
             expect(getPreferenceSpy).toHaveBeenCalledWith('android-targetSdkVersion', 'android');
             expect(target).toBe('android-' + DEFAULT_TARGET_API);