From 1f0ea173b091486e91358fcd9def32a7f516e2ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Raphael=20von=20der=20Gr=C3=BCn?= Date: Tue, 13 Jul 2021 08:51:20 +0200 Subject: [PATCH] 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 --- bin/lib/create.js | 2 +- bin/templates/cordova/Api.js | 4 +- .../cordova/lib/builders/ProjectBuilder.js | 4 +- bin/templates/cordova/lib/check_reqs.js | 27 ++++--- bin/templates/cordova/lib/run.js | 6 +- bin/templates/cordova/lib/target.js | 5 +- spec/e2e/e2e.spec.js | 74 +++++++++++++++++++ spec/e2e/plugin.spec.js | 72 ------------------ spec/unit/builders/ProjectBuilder.spec.js | 8 +- spec/unit/builders/builders.spec.js | 4 +- spec/unit/check_reqs.spec.js | 9 ++- spec/unit/prepare.spec.js | 2 +- spec/unit/run.spec.js | 18 ++++- spec/unit/target.spec.js | 31 ++++---- 14 files changed, 142 insertions(+), 124 deletions(-) create mode 100644 spec/e2e/e2e.spec.js delete mode 100644 spec/e2e/plugin.spec.js diff --git a/bin/lib/create.js b/bin/lib/create.js index 84a1ef30ef..3ebb02451e 100755 --- 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 index 9bd048ce28..4c5e0c5c53 100644 --- 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 index 08e21dab7f..cdc67a92cd 100644 --- 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 index b986375586..8c18db93f2 100644 --- 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 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 index 3a573349be..2a4ff94e47 100644 --- 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 index 2a2d6241bf..8c29788f85 100644 --- 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 new file mode 100644 index 0000000000..1225ccda1e --- /dev/null +++ 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 deleted file mode 100644 index dae97d3bab..0000000000 --- a/spec/e2e/plugin.spec.js +++ /dev/null @@ -1,72 +0,0 @@ -/* - 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'); - -const fakePluginPath = path.join(__dirname, 'fixtures/cordova-plugin-fake'); -const configXmlPath = path.join(__dirname, '../../bin/templates/project/res/xml/config.xml'); - -describe('plugin add', function () { - let tmpDir; - beforeEach(() => { - const tmpDirTemplate = path.join(os.tmpdir(), 'cordova-android-test-'); - tmpDir = fs.realpathSync(fs.mkdtempSync(tmpDirTemplate)); - }); - afterEach(() => { - fs.removeSync(tmpDir); - }); - - it('Test#001 : create project and add a plugin with framework', function () { - const projectname = 'testpluginframework'; - const projectid = 'com.test.plugin.framework'; - - const config = new ConfigParser(configXmlPath); - config.setPackageName(projectid); - config.setName(projectname); - - const projectPath = path.join(tmpDir, projectname); - const pluginInfo = new PluginInfoProvider().get(fakePluginPath); - const noopEvents = new EventEmitter(); - - return Promise.resolve() - .then(() => Api.createPlatform(projectPath, config, {}, noopEvents)) - .then(() => { - // Allow test project to find the `cordova-android` module - fs.ensureSymlinkSync( - path.join(__dirname, '../..'), - path.join(projectPath, 'node_modules/cordova-android'), - 'junction' - ); - - // We need to use the API from the project or some paths break - // TODO remove this and use the API instance returned from - // createPlatform once we fixed the platform - const Api = require(path.join(projectPath, 'cordova/Api.js')); - const api = new Api('android', projectPath, noopEvents); - - return api.addPlugin(pluginInfo); - }); - }, 90000); -}); diff --git a/spec/unit/builders/ProjectBuilder.spec.js b/spec/unit/builders/ProjectBuilder.spec.js index 8cd653dc6b..01f4eb7c30 100644 --- 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 index 64645e1040..e0b72299ef 100644 --- 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 index ac3ba5ba8e..37e00f31ae 100644 --- 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); diff --git a/spec/unit/prepare.spec.js b/spec/unit/prepare.spec.js index 2b6e4af80a..9b8868c40c 100644 --- a/spec/unit/prepare.spec.js +++ b/spec/unit/prepare.spec.js @@ -787,7 +787,7 @@ describe('prepare', () => { gradlePropertiesParserSpy = spyOn(GradlePropertiesParser.prototype, 'configure'); - api = new Api(); + api = new Api('android', cordovaProject.root); }); it('runs without arguments', async () => { diff --git a/spec/unit/run.spec.js b/spec/unit/run.spec.js index da22706557..e440107746 100644 --- a/spec/unit/run.spec.js +++ b/spec/unit/run.spec.js @@ -56,7 +56,8 @@ describe('run', () => { run.__set__({ target: targetSpyObj, - emulator: emulatorSpyObj + emulator: emulatorSpyObj, + AndroidManifest: class {} }); const builder = builders.getBuilder('FakeRootPath'); @@ -66,14 +67,25 @@ describe('run', () => { }); // run needs `this` to behave like an Api instance - run.run = run.run.bind({ _builder: builder }); + run.run = run.run.bind({ + _builder: builder, + locations: { manifest: 'FakeManifestPath' } + }); }); it('should install on target after build', () => { + const AndroidManifest = run.__get__('AndroidManifest'); + return run.run().then(() => { expect(targetSpyObj.install).toHaveBeenCalledWith( resolvedTarget, - { apkPaths: ['fake.apk'], buildType: 'debug' } + { + manifest: jasmine.any(AndroidManifest), + buildResults: { + buildType: 'debug', + apkPaths: ['fake.apk'] + } + } ); }); }); diff --git a/spec/unit/target.spec.js b/spec/unit/target.spec.js index 495bea1035..3ef5c2cad7 100644 --- a/spec/unit/target.spec.js +++ b/spec/unit/target.spec.js @@ -226,25 +226,20 @@ describe('target', () => { }); describe('install', () => { - let AndroidManifestSpy; - let AndroidManifestFns; - let AndroidManifestGetActivitySpy; let AdbSpy; let buildSpy; - let installTarget; + let installTarget, manifest, appSpec; beforeEach(() => { installTarget = { id: 'emulator-5556', type: 'emulator', arch: 'atari' }; + manifest = jasmine.createSpyObj('manifestStub', ['getPackageId', 'getActivity']); + manifest.getActivity.and.returnValue(jasmine.createSpyObj('Activity', ['getName'])); + appSpec = { manifest, buildResults: {} }; + buildSpy = jasmine.createSpyObj('build', ['findBestApkForArchitecture']); target.__set__('build', buildSpy); - AndroidManifestFns = jasmine.createSpyObj('AndroidManifestFns', ['getPackageId', 'getActivity']); - AndroidManifestGetActivitySpy = jasmine.createSpyObj('getActivity', ['getName']); - AndroidManifestFns.getActivity.and.returnValue(AndroidManifestGetActivitySpy); - AndroidManifestSpy = jasmine.createSpy('AndroidManifest').and.returnValue(AndroidManifestFns); - target.__set__('AndroidManifest', AndroidManifestSpy); - AdbSpy = jasmine.createSpyObj('Adb', ['shell', 'start', 'install', 'uninstall']); AdbSpy.shell.and.returnValue(Promise.resolve()); AdbSpy.start.and.returnValue(Promise.resolve()); @@ -257,7 +252,7 @@ describe('target', () => { }); it('should install to the passed target', () => { - return target.install(installTarget, {}).then(() => { + return target.install(installTarget, appSpec).then(() => { expect(AdbSpy.install.calls.argsFor(0)[0]).toBe(installTarget.id); }); }); @@ -272,7 +267,7 @@ describe('target', () => { const apkPath = 'my/apk/path/app.apk'; buildSpy.findBestApkForArchitecture.and.returnValue(apkPath); - return target.install(installTarget, buildResults).then(() => { + return target.install(installTarget, { manifest, buildResults }).then(() => { expect(buildSpy.findBestApkForArchitecture).toHaveBeenCalledWith(buildResults, installTarget.arch); expect(AdbSpy.install.calls.argsFor(0)[1]).toBe(apkPath); @@ -285,7 +280,7 @@ describe('target', () => { Promise.resolve() ); - return target.install(installTarget, {}).then(() => { + return target.install(installTarget, appSpec).then(() => { expect(AdbSpy.install).toHaveBeenCalledTimes(2); expect(AdbSpy.uninstall).toHaveBeenCalled(); }); @@ -295,7 +290,7 @@ describe('target', () => { const errorMsg = 'Failure: Failed to install'; AdbSpy.install.and.rejectWith(new CordovaError(errorMsg)); - return target.install(installTarget, {}).then( + return target.install(installTarget, appSpec).then( () => fail('Unexpectedly resolved'), err => { expect(err).toEqual(jasmine.any(CordovaError)); @@ -305,7 +300,7 @@ describe('target', () => { }); it('should unlock the screen on device', () => { - return target.install(installTarget, {}).then(() => { + return target.install(installTarget, appSpec).then(() => { expect(AdbSpy.shell).toHaveBeenCalledWith(installTarget.id, 'input keyevent 82'); }); }); @@ -313,10 +308,10 @@ describe('target', () => { it('should start the newly installed app on the device', () => { const packageId = 'unittestapp'; const activityName = 'TestActivity'; - AndroidManifestFns.getPackageId.and.returnValue(packageId); - AndroidManifestGetActivitySpy.getName.and.returnValue(activityName); + manifest.getPackageId.and.returnValue(packageId); + manifest.getActivity().getName.and.returnValue(activityName); - return target.install(installTarget, {}).then(() => { + return target.install(installTarget, appSpec).then(() => { expect(AdbSpy.start).toHaveBeenCalledWith(installTarget.id, `${packageId}/.${activityName}`); }); });