[GitHub] [cordova-android] PieterVanPoyer commented on a change in pull request #1229: feat: CORDOVA_JAVA_HOME env variable

[GitBox <gi...@apache.org>]

PieterVanPoyer commented on a change in pull request #1229:
URL: https://github.com/apache/cordova-android/pull/1229#discussion_r628886164



##########
File path: spec/unit/java.spec.js
##########
@@ -76,6 +76,19 @@ describe('Java', () => {
             Java.__set__('javaIsEnsured', false);
         });
 
+        it('CORDOVA_JAVA_HOME overrides JAVA_HOME', async () => {
+            spyOn(utils, 'forgivingWhichSync').and.returnValue('');
+
+            const env = {
+                CORDOVA_JAVA_HOME: '/tmp/jdk'
+            };
+
+            await Java._ensure(env);
+
+            expect(env.PATH.split(path.delimiter))
+                .toContain(path.join(env.JAVA_HOME, 'bin'));

Review comment:
       This is maybe nitpicking. (Feel free to ignore or comment it, off course )
   But I expect the test to be without env.JAVA_HOME . 
   I expect it more like
   ```
   expect(env.PATH.split(path.delimiter)).toContain(path.join('/tmp/jdk', 'bin'));
   ```
   You know exactly what it should be. Assert the exact value.
   Now, when someone changes the business logic around JAVA_HOME and doesn't use the CORDOVA_JAVA_HOME in the correct way, the test could still succeed.




-- 
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org