[GitHub] [maven-surefire] CMoH commented on a change in pull request #387: [SUREFIRE-1939] Fix NPE in SystemUtils.toJdkHomeFromJvmExec if java home has 2 components

2021-12-21 Thread GitBox 


CMoH commented on a change in pull request #387:
URL: https://github.com/apache/maven-surefire/pull/387#discussion_r77382



##
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: issues-unsubscr...@maven.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [maven-surefire] CMoH commented on a change in pull request #387: [SUREFIRE-1939] Fix NPE in SystemUtils.toJdkHomeFromJvmExec if java home has 2 components

2021-11-13 Thread GitBox 


CMoH commented on a change in pull request #387:
URL: https://github.com/apache/maven-surefire/pull/387#discussion_r748766515



##
File path: 
surefire-booter/src/main/java/org/apache/maven/surefire/booter/SystemUtils.java
##
@@ -88,7 +88,7 @@ public static boolean endsWithJavaPath( String jvmExecPath )
 public static File toJdkHomeFromJvmExec( String jvmExecutable )

Review comment:
   I feel the fix is more complete now. Thanks for the comment




-- 
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: issues-unsubscr...@maven.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [maven-surefire] CMoH commented on a change in pull request #387: [SUREFIRE-1939] Fix NPE in SystemUtils.toJdkHomeFromJvmExec if java home has 2 components

2021-11-13 Thread GitBox 


CMoH commented on a change in pull request #387:
URL: https://github.com/apache/maven-surefire/pull/387#discussion_r748766331



##
File path: 
surefire-booter/src/main/java/org/apache/maven/surefire/booter/SystemUtils.java
##
@@ -88,7 +88,7 @@ public static boolean endsWithJavaPath( String jvmExecPath )
 public static File toJdkHomeFromJvmExec( String jvmExecutable )

Review comment:
   No, I didn't feel the need to add a new unit test since the existing 
unit test fails with NPE under the described conditions (i.e JAVA_HOME set to a 
directory 2 levels up from root, `/opt/openjdk-bin-11.0.11_p9` in my case).
   
   However, I have added a system-independent unit test to showcase the 
situation.
   
   




-- 
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: issues-unsubscr...@maven.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org