refactor(env/java): improve tests and implementation (#1246)

This basically fixes up the changes from #1220.

* test(env/java): replace test that duplicates implementation
* test(env/java): stub _ensure to focus on unit under test
* test(env/java): add test for invalid output
* refactor(env/java): keep try block small
* refactor(env/java): shorten excessive comment

bin/templates/cordova/lib/env/java.js

@@ -41,30 +41,9 @@ const java = {
         await java._ensure(process.env);
 
         // Java <= 8 writes version info to stderr, Java >= 9 to stdout
-        let version = null;
+        let javacOutput;
         try {
-            const javacOutput = (await execa('javac', ['-version'], { all: true })).all;
-
-            /*
-            A regex match for the java version looks like the following:
-
-            version: [
-                'javac 1.8.0',
-                '1.8.0',
-                index: 45,
-                input: 'Picked up _JAVA_OPTIONS: -Xms1024M -Xmx2048M\njavac 1.8.0_271',
-                groups: undefined
-            ]
-
-            We have to use a regular expression to get the java version, because on some environments
-            (e.g. macOS Big Sur) javac prints _JAVA_OPTIONS before printing the version and semver.coerce()
-            will fail to get the correct version from the output.
-            */
-
-            const match = /javac\s+([\d.]+)/i.exec(javacOutput);
-            if (match && match[1]) {
-                version = match[1];
-            }
+            javacOutput = (await execa('javac', ['-version'], { all: true })).all;
         } catch (ex) {
             events.emit('verbose', ex.shortMessage);
 
@@ -79,7 +58,11 @@ const java = {
             throw new CordovaError(msg);
         }
 
-        return semver.coerce(version);
+        // We have to filter the output, because javac prints _JAVA_OPTIONS
+        // before printing the version which causes semver.coerce to fail to get
+        // the correct version if those options contain any numbers.
+        const match = /javac\s+([\d.]+)/i.exec(javacOutput);
+        return semver.coerce(match && match[1]);
     },
 
     /**

spec/unit/check_reqs.spec.js

@@ -64,25 +64,6 @@ describe('check_reqs', function () {
 
             await expectAsync(check_reqs.check_java()).toBeResolvedTo({ version: '1.8.0' });
         });
-
-        it('should return the correct version if javac prints _JAVA_OPTIONS', async () => {
-            check_reqs.__set__({
-                java: {
-                    getVersion: async () => {
-                        let version = null;
-                        const javacOutput = 'Picked up _JAVA_OPTIONS: -Xms1024M -Xmx2048M\njavac 1.8.0_271';
-                        const match = /javac\s+([\d.]+)/i.exec(javacOutput);
-                        if (match && match[1]) {
-                            version = match[1];
-                        }
-
-                        return { version };
-                    }
-                }
-            });
-
-            await expectAsync(check_reqs.check_java()).toBeResolvedTo({ version: '1.8.0' });
-        });
     });
 
     describe('check_android', function () {

spec/unit/java.spec.js

@@ -27,19 +27,9 @@ describe('Java', () => {
     const Java = rewire('../../bin/templates/cordova/lib/env/java');
 
     describe('getVersion', () => {
-        let originalEnsureFunc = null;
-
         beforeEach(() => {
-            /*
-                This is to avoid changing/polluting the
-                process environment variables
-                as a result of running these java tests; which could produce
-                unexpected side effects to other tests.
-             */
-            originalEnsureFunc = Java._ensure;
-            spyOn(Java, '_ensure').and.callFake(() => {
-                return originalEnsureFunc({});
-            });
+            // No need to run _ensure, since we are stubbing execa
+            spyOn(Java, '_ensure').and.resolveTo();
         });
 
         it('runs', async () => {
@@ -66,7 +56,16 @@ describe('Java', () => {
             expect(result.version).toBe('1.8.0');
         });
 
-        it('produces a CordovaError on error', async () => {
+        it('detects JDK when additional details contain numbers', async () => {
+            Java.__set__('execa', () => Promise.resolve({
+                all: 'Picked up _JAVA_OPTIONS: -Xms1024M -Xmx2048M\njavac 1.8.0_271'
+            }));
+
+            const { version } = await Java.getVersion();
+            expect(version).toBe('1.8.0');
+        });
+
+        it('produces a CordovaError on subprocess error', async () => {
             Java.__set__('execa', () => Promise.reject({
                 shortMessage: 'test error'
             }));
@@ -79,6 +78,14 @@ describe('Java', () => {
                 .toBeRejectedWithError(CordovaError, /Failed to run "javac -version"/);
             expect(emitSpy).toHaveBeenCalledWith('verbose', 'test error');
         });
+
+        it('throws an error on unexpected output', async () => {
+            Java.__set__('execa', () => Promise.reject({
+                all: '-version not supported'
+            }));
+
+            await expectAsync(Java.getVersion()).toBeRejectedWithError();
+        });
     });
 
     describe('_ensure', () => {