raphinesse commented on a change in pull request #1130:
URL: https://github.com/apache/cordova-android/pull/1130#discussion_r563594432
##########
File path: bin/templates/cordova/lib/check_reqs.js
##########
@@ -20,29 +20,23 @@
const execa = require('execa');
var path = require('path');
var fs = require('fs-extra');
-var os = require('os');
-var which = require('which');
-const glob = require('fast-glob');
+const { forgivingWhichSync, isWindows, isDarwin } = require('./utils');
+const java = require('./env/java');
var REPO_ROOT = path.join(__dirname, '..', '..', '..', '..');
var PROJECT_ROOT = path.join(__dirname, '..', '..');
const { CordovaError, ConfigParser, events } = require('cordova-common');
var android_sdk = require('./android_sdk');
const { createEditor } = require('properties-parser');
+const semver = require('semver');
-function forgivingWhichSync (cmd) {
- const whichResult = which.sync(cmd, { nothrow: true });
-
- // On null, returns empty string to maintain backwards compatibility
- // realpathSync follows symlinks
- return whichResult === null ? '' : fs.realpathSync(whichResult);
-}
+const EXPECTED_JAVA_VERSION = '1.8.x';
module.exports.isWindows = function () {
- return (os.platform() === 'win32');
+ return isWindows();
};
module.exports.isDarwin = function () {
- return (os.platform() === 'darwin');
+ return isDarwin();
};
Review comment:
Do we need the indirection here for faking these functions in tests? If
not, we could shorten these lines to
```js
// Re-export these utilities for <INSERT_REASON> purposes
Object.assign(module.exports, { isWindows, isDarwin });
```
##########
File path: bin/templates/cordova/lib/check_reqs.js
##########
@@ -143,72 +137,20 @@ module.exports.check_gradle = function () {
'in your path, or install Android Studio'));
};
-// Returns a promise.
-module.exports.check_java = function () {
- var javacPath = forgivingWhichSync('javac');
- var hasJavaHome = !!process.env.JAVA_HOME;
- return Promise.resolve().then(function () {
- if (hasJavaHome) {
- // Windows java installer doesn't add javac to PATH, nor set
JAVA_HOME (ugh).
- if (!javacPath) {
- process.env.PATH += path.delimiter +
path.join(process.env.JAVA_HOME, 'bin');
- }
- } else {
- if (javacPath) {
- // OS X has a command for finding JAVA_HOME.
- var find_java = '/usr/libexec/java_home';
- var default_java_error_msg = 'Failed to find \'JAVA_HOME\'
environment variable. Try setting it manually.';
- if (fs.existsSync(find_java)) {
- return execa(find_java).then(({ stdout }) => {
- process.env.JAVA_HOME = stdout;
- }).catch(function (err) {
- if (err) {
- throw new CordovaError(default_java_error_msg);
- }
- });
- } else {
- // See if we can derive it from javac's location.
- var maybeJavaHome = path.dirname(path.dirname(javacPath));
- if (fs.existsSync(path.join(maybeJavaHome, 'lib',
'tools.jar'))) {
- process.env.JAVA_HOME = maybeJavaHome;
- } else {
- throw new CordovaError(default_java_error_msg);
- }
- }
- } else if (module.exports.isWindows()) {
- const { env } = process;
- const baseDirs = [env.ProgramFiles, env['ProgramFiles(x86)']];
- const globOpts = { absolute: true, onlyDirectories: true };
- const flatMap = (arr, f) => [].concat(...arr.map(f));
-
- const jdkDir = flatMap(baseDirs, cwd =>
- glob.sync('java/jdk*', { cwd, ...globOpts })
- )[0];
-
- if (jdkDir) {
- env.PATH += path.delimiter + path.join(jdkDir, 'bin');
- env.JAVA_HOME = path.normalize(jdkDir);
- }
- }
- }
- }).then(function () {
- return execa('javac', ['-version'], { all: true })
- .then(({ all: output }) => {
- // Java <= 8 writes version info to stderr, Java >= 9 to stdout
- const match = /javac\s+([\d.]+)/i.exec(output);
- return match && match[1];
- }, () => {
- var msg =
- 'Failed to run "javac -version", make sure that you have a JDK
version 8 installed.\n' +
- 'You can get it from the following location:\n' +
-
'https://www.oracle.com/technetwork/java/javase/downloads/jdk8-downloads-2133151.html';
- if (process.env.JAVA_HOME) {
- msg += '\n\n';
- msg += 'Your JAVA_HOME is invalid: ' +
process.env.JAVA_HOME;
- }
- throw new CordovaError(msg);
- });
- });
+/**
+ * Checks for the java installation and correct version
+ */
+module.exports.check_java = async function () {
+ const javaVersion = await java.getVersion();
+
+ if (!semver.satisfies(javaVersion.version, `= ${EXPECTED_JAVA_VERSION}`)) {
+ throw new CordovaError(
+ `Requirements check failed for JDK ${EXPECTED_JAVA_VERSION}!
Detected version: ${javaVersion.version}\n` +
+ 'Check your ANDROID_SDK_ROOT / JAVA_HOME / PATH environment
variables.'
+ );
+ }
+
+ return javaVersion.string;
Review comment:
The return value does not seem to be used now. Will it be used by the
upcoming feature?
##########
File path: bin/templates/cordova/lib/check_reqs.js
##########
@@ -143,72 +137,20 @@ module.exports.check_gradle = function () {
'in your path, or install Android Studio'));
};
-// Returns a promise.
-module.exports.check_java = function () {
- var javacPath = forgivingWhichSync('javac');
- var hasJavaHome = !!process.env.JAVA_HOME;
- return Promise.resolve().then(function () {
- if (hasJavaHome) {
- // Windows java installer doesn't add javac to PATH, nor set
JAVA_HOME (ugh).
- if (!javacPath) {
- process.env.PATH += path.delimiter +
path.join(process.env.JAVA_HOME, 'bin');
- }
- } else {
- if (javacPath) {
- // OS X has a command for finding JAVA_HOME.
- var find_java = '/usr/libexec/java_home';
- var default_java_error_msg = 'Failed to find \'JAVA_HOME\'
environment variable. Try setting it manually.';
- if (fs.existsSync(find_java)) {
- return execa(find_java).then(({ stdout }) => {
- process.env.JAVA_HOME = stdout;
- }).catch(function (err) {
- if (err) {
- throw new CordovaError(default_java_error_msg);
- }
- });
- } else {
- // See if we can derive it from javac's location.
- var maybeJavaHome = path.dirname(path.dirname(javacPath));
- if (fs.existsSync(path.join(maybeJavaHome, 'lib',
'tools.jar'))) {
- process.env.JAVA_HOME = maybeJavaHome;
- } else {
- throw new CordovaError(default_java_error_msg);
- }
- }
- } else if (module.exports.isWindows()) {
- const { env } = process;
- const baseDirs = [env.ProgramFiles, env['ProgramFiles(x86)']];
- const globOpts = { absolute: true, onlyDirectories: true };
- const flatMap = (arr, f) => [].concat(...arr.map(f));
-
- const jdkDir = flatMap(baseDirs, cwd =>
- glob.sync('java/jdk*', { cwd, ...globOpts })
- )[0];
-
- if (jdkDir) {
- env.PATH += path.delimiter + path.join(jdkDir, 'bin');
- env.JAVA_HOME = path.normalize(jdkDir);
- }
- }
- }
- }).then(function () {
- return execa('javac', ['-version'], { all: true })
- .then(({ all: output }) => {
- // Java <= 8 writes version info to stderr, Java >= 9 to stdout
- const match = /javac\s+([\d.]+)/i.exec(output);
- return match && match[1];
- }, () => {
- var msg =
- 'Failed to run "javac -version", make sure that you have a JDK
version 8 installed.\n' +
- 'You can get it from the following location:\n' +
-
'https://www.oracle.com/technetwork/java/javase/downloads/jdk8-downloads-2133151.html';
- if (process.env.JAVA_HOME) {
- msg += '\n\n';
- msg += 'Your JAVA_HOME is invalid: ' +
process.env.JAVA_HOME;
- }
- throw new CordovaError(msg);
- });
- });
+/**
+ * Checks for the java installation and correct version
+ */
+module.exports.check_java = async function () {
+ const javaVersion = await java.getVersion();
+
+ if (!semver.satisfies(javaVersion.version, `= ${EXPECTED_JAVA_VERSION}`)) {
Review comment:
I _think_ this can be shortened to
```suggestion
if (!semver.satisfies(javaVersion.version, EXPECTED_JAVA_VERSION)) {
```
##########
File path: spec/unit/check_reqs.spec.js
##########
@@ -48,33 +48,15 @@ describe('check_reqs', function () {
});
describe('check_java', () => {
- let tmpDir;
- beforeEach(() => {
- const tmpDirTemplate = path.join(os.tmpdir(),
'cordova-android-test-');
- tmpDir = fs.realpathSync(fs.mkdtempSync(tmpDirTemplate));
- });
- afterEach(() => {
- fs.removeSync(tmpDir);
- });
-
- it('detects JDK in default location on windows', async () => {
- check_reqs.isWindows = () => true;
- check_reqs.__set__({
- execa: async () => ({}),
- forgivingWhichSync: () => ''
- });
-
- delete process.env.JAVA_HOME;
- process.env.ProgramFiles = tmpDir;
-
- const jdkDir = path.join(tmpDir, 'java/jdk1.6.0_02');
- fs.ensureDirSync(jdkDir);
+ it('detects if unexpected JDK version is installed', async () => {
+ check_reqs.__set__('EXPECTED_JAVA_VERSION', '9999.9999.9999');
+ const java = require('../../bin/templates/cordova/lib/env/java');
- await check_reqs.check_java();
+ spyOn(java, 'getVersion').and.returnValue(Promise.resolve({
+ version: '1.8.0'
+ }));
Review comment:
```suggestion
```
we can omit this if we stub the module above
##########
File path: spec/unit/java.spec.js
##########
@@ -0,0 +1,158 @@
+/**
+ 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 path = require('path');
+const rewire = require('rewire');
+const { CordovaError } = require('cordova-common');
+const utils = require('../../bin/templates/cordova/lib/utils');
+const glob = require('fast-glob');
+
+describe('Java', () => {
+ const Java = rewire('../../bin/templates/cordova/lib/env/java');
+
+ describe('getVersion', () => {
Review comment:
I think these tests should stub `Java._ensure`. If not, these tests
might be altering the global `process.env`.
(Plus _maybe_ add a test that shows that `Java._ensure` is called).
##########
File path: spec/unit/java.spec.js
##########
@@ -0,0 +1,158 @@
+/**
+ 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 path = require('path');
+const rewire = require('rewire');
+const { CordovaError } = require('cordova-common');
+const utils = require('../../bin/templates/cordova/lib/utils');
+const glob = require('fast-glob');
+
+describe('Java', () => {
+ const Java = rewire('../../bin/templates/cordova/lib/env/java');
+
+ describe('getVersion', () => {
+ it('runs', async () => {
+ Java.__set__('execa', () => Promise.resolve({
+ all: 'javac 1.8.0_275'
+ }));
+
+ const result = await Java.getVersion();
+ expect(result.major).toBe(1);
+ expect(result.minor).toBe(8);
+ expect(result.patch).toBe(0);
+ expect(result.version).toBe('1.8.0');
+ });
+
+ it('produces a CordovaError on error', async () => {
+ Java.__set__('execa', () => Promise.reject({
+ shortMessage: 'test error'
+ }));
+ const emitSpy = jasmine.createSpy('events.emit');
+ Java.__set__('events', {
+ emit: emitSpy
+ });
+
+ await
expectAsync(Java.getVersion()).toBeRejectedWithError(CordovaError, /Failed to
run "javac -version"/);
+ expect(emitSpy).toHaveBeenCalledWith('verbose', 'test error');
+ });
+ });
+
+ describe('_ensure', () => {
+ beforeEach(() => {
+ Java.__set__('javaIsEnsured', false);
+ });
+
+ it('with JAVA_HOME / without javac', async () => {
+ spyOn(utils, 'forgivingWhichSync').and.returnValue('');
+
+ const env = {
+ JAVA_HOME: '/tmp/jdk'
+ };
+
+ await Java._ensure(env);
+
+ expect(env.PATH.split(path.delimiter))
+ .toContain(path.join(env.JAVA_HOME, 'bin'));
+ });
+
+ it('detects JDK in default location on windows', async () => {
+ spyOn(utils, 'forgivingWhichSync').and.returnValue('');
+ spyOn(utils, 'isWindows').and.returnValue(true);
+
+ const root = 'C:\\Program Files';
+ const env = {
+ ProgramFiles: root
+ };
+
+ spyOn(glob, 'sync').and.returnValue(`${root}\\java\\jdk1.8.0_275`);
+
+ const jdkDir = `${root}\\java\\jdk1.8.0_275`;
+
+ await Java._ensure(env);
+
+ expect(env.JAVA_HOME).withContext('JAVA_HOME').toBe(jdkDir);
+ expect(env.PATH).toContain(jdkDir);
+ });
+
+ it('detects JDK in default location on windows (x86)', async () => {
+ spyOn(utils, 'forgivingWhichSync').and.returnValue('');
+ spyOn(utils, 'isWindows').and.returnValue(true);
+
+ const root = 'C:\\Program Files (x86)';
+ const env = {
+ 'ProgramFiles(x86)': root
+ };
+
+ spyOn(glob, 'sync').and.returnValue(`${root}\\java\\jdk1.8.0_275`);
+
+ const jdkDir = `${root}\\java\\jdk1.8.0_275`;
+
+ await Java._ensure(env);
+
+ expect(env.JAVA_HOME).withContext('JAVA_HOME').toBe(jdkDir);
+ expect(env.PATH).toContain(jdkDir);
+ });
+
+ it('without JAVA_HOME / with javac - Mac OS X - success', async () => {
+ spyOn(utils, 'forgivingWhichSync').and.returnValue('/tmp/jdk/bin');
+ const fsSpy = jasmine.createSpy('fs').and.returnValue(true);
+ Java.__set__('fs', {
+ existsSync: fsSpy
+ });
+ Java.__set__('execa', async () => ({ stdout: '/tmp/jdk' }));
+
+ const env = {};
+
+ await Java._ensure(env);
+
+ expect(fsSpy).toHaveBeenCalledWith('/usr/libexec/java_home');
+ expect(env.JAVA_HOME).toBe('/tmp/jdk');
+ });
+
+ it('without JAVA_HOME / with javac - Mac OS X - error', async () => {
+ spyOn(utils, 'forgivingWhichSync').and.returnValue('/tmp/jdk/bin');
+ Java.__set__('fs', { existsSync: () => true });
+ Java.__set__('execa',
jasmine.createSpy('execa').and.returnValue(Promise.reject({
+ shortMessage: 'test error'
+ })));
+ const emitSpy = jasmine.createSpy('events.emit');
+ Java.__set__('events', {
+ emit: emitSpy
+ });
+
+ await
expectAsync(Java._ensure({})).toBeRejectedWithError(CordovaError, /Failed to
find 'JAVA_HOME' environment variable/);
Review comment:
Cosmetic
```suggestion
await expectAsync(Java._ensure({}))
.toBeRejectedWithError(CordovaError, /Failed to find
'JAVA_HOME' environment variable/);
```
##########
File path: spec/unit/check_reqs.spec.js
##########
@@ -48,33 +48,15 @@ describe('check_reqs', function () {
});
describe('check_java', () => {
- let tmpDir;
- beforeEach(() => {
- const tmpDirTemplate = path.join(os.tmpdir(),
'cordova-android-test-');
- tmpDir = fs.realpathSync(fs.mkdtempSync(tmpDirTemplate));
- });
- afterEach(() => {
- fs.removeSync(tmpDir);
- });
-
- it('detects JDK in default location on windows', async () => {
- check_reqs.isWindows = () => true;
- check_reqs.__set__({
- execa: async () => ({}),
- forgivingWhichSync: () => ''
- });
-
- delete process.env.JAVA_HOME;
- process.env.ProgramFiles = tmpDir;
-
- const jdkDir = path.join(tmpDir, 'java/jdk1.6.0_02');
- fs.ensureDirSync(jdkDir);
+ it('detects if unexpected JDK version is installed', async () => {
+ check_reqs.__set__('EXPECTED_JAVA_VERSION', '9999.9999.9999');
+ const java = require('../../bin/templates/cordova/lib/env/java');
Review comment:
No need to require `env/java` just to stub `getVersion`. We might as
well stub the whole module:
```suggestion
check_reqs.__set__({
EXPECTED_JAVA_VERSION: '9999.9999.9999',
java: { getVersion: async () => ({ version: '1.8.0' }) }
});
```
##########
File path: bin/templates/cordova/lib/env/java.js
##########
@@ -0,0 +1,119 @@
+/*
+ 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 execa = require('execa');
+const fs = require('fs-extra');
+const path = require('path');
+const glob = require('fast-glob');
+const { CordovaError, events } = require('cordova-common');
+const utils = require('../utils');
+const semver = require('semver');
+
+let javaIsEnsured = false;
+
+const java = {
+ /**
+ * Gets the version from the javac executable.
+ *
+ * @returns {semver.SemVer}
+ */
+ getVersion: async () => {
+ await java._ensure(process.env);
+
+ // Java <= 8 writes version info to stderr, Java >= 9 to stdout
+ let version = null;
+ try {
+ version = (await execa('javac', ['-version'], { all: true })).all;
+ } catch (ex) {
+ events.emit('verbose', ex.shortMessage);
+
+ let msg =
+ 'Failed to run "javac -version", make sure that you have a JDK
version 8 installed.\n' +
+ 'You can get it from the following location:\n' +
+
'https://www.oracle.com/technetwork/java/javase/downloads/jdk8-downloads-2133151.html';
+ if (process.env.JAVA_HOME) {
+ msg += '\n\n';
+ msg += 'Your JAVA_HOME is invalid: ' + process.env.JAVA_HOME;
+ }
+ throw new CordovaError(msg);
+ }
+
+ return semver.coerce(version);
+ },
+
+ /**
+ * Ensures that Java is installed. Will throw exception if not.
+ * Will set JAVA_HOME and PATH environment variables.
+ *
+ * This function becomes a no-op if already ran previously.
+ */
+ _ensure: async (environment) => {
+ if (javaIsEnsured) {
+ return;
+ }
+
+ const javacPath = utils.forgivingWhichSync('javac');
+ const hasJavaHome = !!environment.JAVA_HOME;
+ if (hasJavaHome) {
+ // Windows java installer doesn't add javac to PATH, nor set
JAVA_HOME (ugh).
+ if (!javacPath) {
+ environment.PATH += path.delimiter +
path.join(environment.JAVA_HOME, 'bin');
+ }
+ } else {
+ if (javacPath) {
+ // OS X has a command for finding JAVA_HOME.
+ const find_java = '/usr/libexec/java_home';
+ const default_java_error_msg = 'Failed to find \'JAVA_HOME\'
environment variable. Try setting it manually.';
+ if (fs.existsSync(find_java)) {
+ try {
+ environment.JAVA_HOME = (await
execa(find_java)).stdout;
+ } catch (ex) {
+ events.emit('verbose', ex.shortMessage);
+ throw new CordovaError(default_java_error_msg);
+ }
+ } else {
+ // See if we can derive it from javac's location.
+ var maybeJavaHome = path.dirname(path.dirname(javacPath));
+ if (fs.existsSync(path.join(maybeJavaHome, 'lib',
'tools.jar'))) {
+ environment.JAVA_HOME = maybeJavaHome;
+ } else {
+ throw new CordovaError(default_java_error_msg);
+ }
+ }
+ } else if (utils.isWindows()) {
+ const baseDirs = [environment.ProgramFiles,
environment['ProgramFiles(x86)']];
+ const globOpts = { absolute: true, onlyDirectories: true };
+ const flatMap = (arr, f) => [].concat(...arr.map(f));
+ const jdkDir = flatMap(baseDirs, cwd => {
+ return glob.sync('java/jdk*', { cwd, ...globOpts });
+ }
+ )[0];
+
+ if (jdkDir) {
+ environment.PATH += path.delimiter + path.join(jdkDir,
'bin');
+ environment.JAVA_HOME = path.normalize(jdkDir);
+ }
+ }
+ }
+
+ javaIsEnsured = true;
Review comment:
I assume this flag is used to prevent running the expensive synchronous
operations in this method multiple times. I think this is basically a good
idea, but I'm not so sure about the execution.
My problem here is that we only set this flag if we succeeded. If an
exception was thrown, we don't get to this point and will execute the function
regularly on subsequent calls to it. I'm just not sure this is what we actually
want.
Seeing how the function to be cached is `async`, a wrapper that caches both
successes and failures would be easy to implement:
```js
const doEnsure = environment => {
// same as the current _ensure, just without the javaIsEnsured bits
};
const cacheAsyncFunction = f => {
let cachedResult;
return (...args) => cachedResult || (cachedResult = f(...args));
};
// run doEnsure on first invocation, return cached result afterwards
const _ensure = cacheAsyncFunction(doEnsure);
```
What do you think is the preferable behavior here?
##########
File path: spec/unit/java.spec.js
##########
@@ -0,0 +1,158 @@
+/**
+ 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 path = require('path');
+const rewire = require('rewire');
+const { CordovaError } = require('cordova-common');
+const utils = require('../../bin/templates/cordova/lib/utils');
+const glob = require('fast-glob');
+
+describe('Java', () => {
+ const Java = rewire('../../bin/templates/cordova/lib/env/java');
+
+ describe('getVersion', () => {
+ it('runs', async () => {
+ Java.__set__('execa', () => Promise.resolve({
+ all: 'javac 1.8.0_275'
+ }));
+
+ const result = await Java.getVersion();
+ expect(result.major).toBe(1);
+ expect(result.minor).toBe(8);
+ expect(result.patch).toBe(0);
+ expect(result.version).toBe('1.8.0');
+ });
+
+ it('produces a CordovaError on error', async () => {
+ Java.__set__('execa', () => Promise.reject({
+ shortMessage: 'test error'
+ }));
+ const emitSpy = jasmine.createSpy('events.emit');
+ Java.__set__('events', {
+ emit: emitSpy
+ });
+
+ await
expectAsync(Java.getVersion()).toBeRejectedWithError(CordovaError, /Failed to
run "javac -version"/);
Review comment:
Cosmetic
```suggestion
await expectAsync(Java.getVersion())
.toBeRejectedWithError(CordovaError, /Failed to run "javac
-version"/);
```
##########
File path: spec/unit/check_reqs.spec.js
##########
@@ -48,33 +48,15 @@ describe('check_reqs', function () {
});
describe('check_java', () => {
- let tmpDir;
- beforeEach(() => {
- const tmpDirTemplate = path.join(os.tmpdir(),
'cordova-android-test-');
- tmpDir = fs.realpathSync(fs.mkdtempSync(tmpDirTemplate));
- });
- afterEach(() => {
- fs.removeSync(tmpDir);
- });
-
- it('detects JDK in default location on windows', async () => {
- check_reqs.isWindows = () => true;
- check_reqs.__set__({
- execa: async () => ({}),
- forgivingWhichSync: () => ''
- });
-
- delete process.env.JAVA_HOME;
- process.env.ProgramFiles = tmpDir;
-
- const jdkDir = path.join(tmpDir, 'java/jdk1.6.0_02');
- fs.ensureDirSync(jdkDir);
+ it('detects if unexpected JDK version is installed', async () => {
+ check_reqs.__set__('EXPECTED_JAVA_VERSION', '9999.9999.9999');
+ const java = require('../../bin/templates/cordova/lib/env/java');
- await check_reqs.check_java();
+ spyOn(java, 'getVersion').and.returnValue(Promise.resolve({
+ version: '1.8.0'
+ }));
- expect(process.env.JAVA_HOME).toBe(jdkDir);
- expect(process.env.PATH.split(path.delimiter))
- .toContain(path.join(jdkDir, 'bin'));
+ await
expectAsync(check_reqs.check_java()).toBeRejectedWithError(CordovaError,
/Requirements check failed for JDK 9999.9999.9999! Detected version: 1.8.0/);
Review comment:
Cosmetic line break
```suggestion
await expectAsync(check_reqs.check_java())
.toBeRejectedWithError(CordovaError, /Requirements check
failed for JDK 9999.9999.9999! Detected version: 1.8.0/);
```
##########
File path: bin/templates/cordova/lib/utils.js
##########
@@ -53,3 +55,19 @@ exports.compareByAll = fns => {
return 0;
};
};
+
+exports.forgivingWhichSync = (cmd) => {
+ const whichResult = which.sync(cmd, { nothrow: true });
+
+ // On null, returns empty string to maintain backwards compatibility
+ // realpathSync follows symlinks
+ return whichResult === null ? '' : fs.realpathSync(whichResult);
+};
+
+exports.isWindows = function () {
+ return (os.platform() === 'win32');
+};
+
+exports.isDarwin = function () {
+ return (os.platform() === 'darwin');
+};
Review comment:
We could shorten this to the following:
```suggestion
exports.isWindows = () => os.platform() === 'win32';
exports.isDarwin = () => os.platform() === 'darwin';
```
purely cosmetic change of course
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
