CMoH commented on a change in pull request #387:
URL: https://github.com/apache/maven-surefire/pull/387#discussion_r773444482
##########
File path:
surefire-booter/src/test/java/org/apache/maven/surefire/booter/SystemUtilsTest.java
##########
@@ -98,6 +98,15 @@ public void incorrectJdkPath()
assertThat( SystemUtils.isJava9AtLeast(
incorrect.getAbsolutePath() ) ).isFalse();
}
+ @Test
+ public void incorrectJdkPathShouldNotNPE()
Review comment:
Well, the reason I submitted this is because unit tests fail, due to
this: on my machine JAVA_HOME is `/opt/openjdk-bin-11/`. Unit test
`incorrectJdkPath()` works as follows:
```
@Test
public void incorrectJdkPath()
{
File jre = new File( System.getProperty( "java.home" ) );
File jdk = jre.getParentFile();
File incorrect = jdk.getParentFile();
assertThat( SystemUtils.isJava9AtLeast(
incorrect.getAbsolutePath() ) ).isFalse();
}
```
This leads to `incorrect=/`, which reaches `toJdkHomeFromJvmExec("/")`,
which calls again `getParent("/")`, which yields null, and next `getName()` is
called on this null. It is clear from the implementation of
`toJdkHomeFromJvmExec()` that returning null is fine when this jvmExec is not
found. I believe this is the case here.
Also, the test is about incorrect JDK paths, and such a path is an incorrect
JDK path. However, one incorrect case crashes with a non-informative NPE. I
would say a "cannot find jvm executable" error on the return-null path would be
more helpful for users that misconfigured their systems.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
