You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2020/04/28 00:29:11 UTC
[GitHub] [maven-surefire] akomakom opened a new pull request #289: SUREFIRE-1784 Set java home from jvm
akomakom opened a new pull request #289:
URL: https://github.com/apache/maven-surefire/pull/289
Similar to #288 but for the jvm parameter.
----------------------------------------------------------------
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #289: SUREFIRE-1784 Fork JVM defined by jvm param should not inherit JAVA_HOME from Maven process
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #289:
URL: https://github.com/apache/maven-surefire/pull/289#discussion_r416878759
##########
File path: maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/AbstractSurefireMojoTest.java
##########
@@ -2183,6 +2183,13 @@ public void setToolchain( Toolchain toolchain ) throws Exception
toolchainField.setAccessible( true );
toolchainField.set( this, toolchain );
}
+
+ public void setJvm( String jvm ) throws Exception
+ {
+ Field toolchainField = AbstractSurefireMojo.class.getDeclaredField( "jvm" );
Review comment:
pls substitute these three lines with `setInternalState( this, "jvm", jvm );`
----------------------------------------------------------------
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
[GitHub] [maven-surefire] Tibor17 commented on pull request #289: SUREFIRE-1784 Fork JVM defined by jvm param should not inherit JAVA_HOME from Maven process
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #289:
URL: https://github.com/apache/maven-surefire/pull/289#issuecomment-620847707
@akomakom
I could not find your email but mine is available in public. Can you contact me? I want to speak with you. Thx
----------------------------------------------------------------
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
[GitHub] [maven-surefire] akomakom commented on a change in pull request #289: SUREFIRE-1784 Fork JVM defined by jvm param should not inherit JAVA_HOME from Maven process
Posted by GitBox <gi...@apache.org>.
akomakom commented on a change in pull request #289:
URL: https://github.com/apache/maven-surefire/pull/289#discussion_r416565840
##########
File path: maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/AbstractSurefireMojoToolchainsTest.java
##########
@@ -154,6 +157,36 @@ public void shouldChangeJavaHomeFromToolchain() throws Exception
assertThat( mojo.getEnvironmentVariables() ).includes( MapAssert.entry( "JAVA_HOME", "/some/path" ) );
}
+ /**
+ * Ensures that the environmentVariables map for launching a test jvm
+ * contains a jvm parameter-driven entry when jvm is set.
+ */
+ @Test
+ public void shouldChangeJavaHomeFromJvm() throws Exception
+ {
+ AbstractSurefireMojoTest.Mojo mojo = new AbstractSurefireMojoTest.Mojo();
+ String fakeJava = "/some/path/to/jdk/bin/java";
+ mojo.setJvm( fakeJava );
+
+ //Set up a mocked File-object
+ File mockedFile = mock( File.class );
+ when( mockedFile.getAbsoluteFile() ).thenReturn( mockedFile );
+ when( mockedFile.getPath() ).thenReturn( fakeJava );
+ when( mockedFile.getName() ).thenReturn( "java" );
+ when( mockedFile.isFile() ).thenReturn( true );
+
+ //Trap constructor calls to return the mocked File-object
+ whenNew( File.class ).
+ withParameterTypes( String.class ).
+ withArguments( anyString() ).
+ thenReturn( mockedFile );
+
+ assertThat( mojo.getEnvironmentVariables() ).isEmpty();
+ invokeMethod( mojo, "getEffectiveJvm" );
Review comment:
:+1:
----------------------------------------------------------------
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
[GitHub] [maven-surefire] akomakom commented on a change in pull request #289: SUREFIRE-1784 Fork JVM defined by jvm param should not inherit JAVA_HOME from Maven process
Posted by GitBox <gi...@apache.org>.
akomakom commented on a change in pull request #289:
URL: https://github.com/apache/maven-surefire/pull/289#discussion_r416898348
##########
File path: maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/AbstractSurefireMojoTest.java
##########
@@ -2183,6 +2183,13 @@ public void setToolchain( Toolchain toolchain ) throws Exception
toolchainField.setAccessible( true );
toolchainField.set( this, toolchain );
}
+
+ public void setJvm( String jvm ) throws Exception
+ {
+ Field toolchainField = AbstractSurefireMojo.class.getDeclaredField( "jvm" );
Review comment:
That's much better.
----------------------------------------------------------------
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
[GitHub] [maven-surefire] Tibor17 commented on pull request #289: SUREFIRE-1784 Fork JVM defined by jvm param should not inherit JAVA_HOME from Maven process
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #289:
URL: https://github.com/apache/maven-surefire/pull/289#issuecomment-620847124
Thx for contributing!
----------------------------------------------------------------
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
[GitHub] [maven-surefire] akomakom commented on pull request #289: SUREFIRE-1784 Fork JVM defined by jvm param should not inherit JAVA_HOME from Maven process
Posted by GitBox <gi...@apache.org>.
akomakom commented on pull request #289:
URL: https://github.com/apache/maven-surefire/pull/289#issuecomment-621164487
> @akomakom
> I could not find your email but mine is available in public. Can you contact me? I want to speak with you. Thx
Emailed you at apache.org
----------------------------------------------------------------
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
[GitHub] [maven-surefire] akomakom commented on pull request #289: SUREFIRE-1784 Set java home from jvm
Posted by GitBox <gi...@apache.org>.
akomakom commented on pull request #289:
URL: https://github.com/apache/maven-surefire/pull/289#issuecomment-620308686
@Tibor17 this is based on the 288 branch, I'll rebase after that's merged.
I couldn't figure out how to test this... I'd have to mock File.isFile() which seems heavy handed.
----------------------------------------------------------------
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
[GitHub] [maven-surefire] akomakom edited a comment on pull request #289: SUREFIRE-1784 Fork JVM defined by jvm param should not inherit JAVA_HOME from Maven process
Posted by GitBox <gi...@apache.org>.
akomakom edited a comment on pull request #289:
URL: https://github.com/apache/maven-surefire/pull/289#issuecomment-620308686
@Tibor17 this is based on the 288 branch, I'll rebase after that's merged.
~~I couldn't figure out how to test this... I'd have to mock File.isFile() which seems heavy handed.~~
----------------------------------------------------------------
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #289: SUREFIRE-1784 Fork JVM defined by jvm param should not inherit JAVA_HOME from Maven process
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #289:
URL: https://github.com/apache/maven-surefire/pull/289#discussion_r416880320
##########
File path: maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/AbstractSurefireMojoTest.java
##########
@@ -2183,6 +2183,13 @@ public void setToolchain( Toolchain toolchain ) throws Exception
toolchainField.setAccessible( true );
toolchainField.set( this, toolchain );
}
+
+ public void setJvm( String jvm ) throws Exception
+ {
+ Field toolchainField = AbstractSurefireMojo.class.getDeclaredField( "jvm" );
Review comment:
It becomes static import `import static org.powermock.reflect.Whitebox.setInternalState;`
----------------------------------------------------------------
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
[GitHub] [maven-surefire] Tibor17 commented on pull request #289: SUREFIRE-1784 Fork JVM defined by jvm param should not inherit JAVA_HOME from Maven process
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #289:
URL: https://github.com/apache/maven-surefire/pull/289#issuecomment-620841393
Thx, we are ready to go.
----------------------------------------------------------------
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #289: SUREFIRE-1784 Fork JVM defined by jvm param should not inherit JAVA_HOME from Maven process
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #289:
URL: https://github.com/apache/maven-surefire/pull/289#discussion_r416446480
##########
File path: maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/AbstractSurefireMojoToolchainsTest.java
##########
@@ -154,6 +157,36 @@ public void shouldChangeJavaHomeFromToolchain() throws Exception
assertThat( mojo.getEnvironmentVariables() ).includes( MapAssert.entry( "JAVA_HOME", "/some/path" ) );
}
+ /**
+ * Ensures that the environmentVariables map for launching a test jvm
+ * contains a jvm parameter-driven entry when jvm is set.
+ */
+ @Test
+ public void shouldChangeJavaHomeFromJvm() throws Exception
+ {
+ AbstractSurefireMojoTest.Mojo mojo = new AbstractSurefireMojoTest.Mojo();
+ String fakeJava = "/some/path/to/jdk/bin/java";
+ mojo.setJvm( fakeJava );
+
+ //Set up a mocked File-object
+ File mockedFile = mock( File.class );
+ when( mockedFile.getAbsoluteFile() ).thenReturn( mockedFile );
+ when( mockedFile.getPath() ).thenReturn( fakeJava );
+ when( mockedFile.getName() ).thenReturn( "java" );
+ when( mockedFile.isFile() ).thenReturn( true );
+
+ //Trap constructor calls to return the mocked File-object
+ whenNew( File.class ).
+ withParameterTypes( String.class ).
+ withArguments( anyString() ).
+ thenReturn( mockedFile );
+
+ assertThat( mojo.getEnvironmentVariables() ).isEmpty();
+ invokeMethod( mojo, "getEffectiveJvm" );
+ assertThat( mojo.getEnvironmentVariables() ).includes( MapAssert.entry( "JAVA_HOME", "/some/path/to/jdk" ) );
Review comment:
The Windows fails with
`java.lang.AssertionError: the map:<{'JAVA_HOME'='D:\some\path\to\jdk'}> does not contain the entry:<['JAVA_HOME'='/some/path/to/jdk']>`
What about to do it a bit of easier and use the real path taken from system property `java.home` which is JRE. It would be the value for `String fakeJava`. Then we would not have to mock the `File`.
It would be worth to write the second test where the `environmentVariables` has already some other path and the assertion statement would verify that the `environmentVariables` is intact.
----------------------------------------------------------------
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #289: SUREFIRE-1784 Fork JVM defined by jvm param should not inherit JAVA_HOME from Maven process
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #289:
URL: https://github.com/apache/maven-surefire/pull/289#discussion_r416448777
##########
File path: maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/AbstractSurefireMojoToolchainsTest.java
##########
@@ -154,6 +157,36 @@ public void shouldChangeJavaHomeFromToolchain() throws Exception
assertThat( mojo.getEnvironmentVariables() ).includes( MapAssert.entry( "JAVA_HOME", "/some/path" ) );
}
+ /**
+ * Ensures that the environmentVariables map for launching a test jvm
+ * contains a jvm parameter-driven entry when jvm is set.
+ */
+ @Test
+ public void shouldChangeJavaHomeFromJvm() throws Exception
+ {
+ AbstractSurefireMojoTest.Mojo mojo = new AbstractSurefireMojoTest.Mojo();
+ String fakeJava = "/some/path/to/jdk/bin/java";
+ mojo.setJvm( fakeJava );
+
+ //Set up a mocked File-object
+ File mockedFile = mock( File.class );
+ when( mockedFile.getAbsoluteFile() ).thenReturn( mockedFile );
+ when( mockedFile.getPath() ).thenReturn( fakeJava );
+ when( mockedFile.getName() ).thenReturn( "java" );
+ when( mockedFile.isFile() ).thenReturn( true );
+
+ //Trap constructor calls to return the mocked File-object
+ whenNew( File.class ).
+ withParameterTypes( String.class ).
+ withArguments( anyString() ).
+ thenReturn( mockedFile );
+
+ assertThat( mojo.getEnvironmentVariables() ).isEmpty();
+ invokeMethod( mojo, "getEffectiveJvm" );
Review comment:
It would be nice to add an assertion statement for returned value and check the value in `JdkAttributes#getJvmExecutable()`. The point of the test is to be very sensitive to a change and fail, that's why more and more assertions are employed.
----------------------------------------------------------------
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
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #289: SUREFIRE-1784 Fork JVM defined by jvm param should not inherit JAVA_HOME from Maven process
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #289:
URL: https://github.com/apache/maven-surefire/pull/289#discussion_r416879210
##########
File path: maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/AbstractSurefireMojoTest.java
##########
@@ -2183,6 +2183,13 @@ public void setToolchain( Toolchain toolchain ) throws Exception
toolchainField.setAccessible( true );
Review comment:
pls substitute these three lines with `setInternalState( this, "toolchain", toolchain );`
----------------------------------------------------------------
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
[GitHub] [maven-surefire] akomakom commented on a change in pull request #289: SUREFIRE-1784 Fork JVM defined by jvm param should not inherit JAVA_HOME from Maven process
Posted by GitBox <gi...@apache.org>.
akomakom commented on a change in pull request #289:
URL: https://github.com/apache/maven-surefire/pull/289#discussion_r416565218
##########
File path: maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/AbstractSurefireMojoToolchainsTest.java
##########
@@ -154,6 +157,36 @@ public void shouldChangeJavaHomeFromToolchain() throws Exception
assertThat( mojo.getEnvironmentVariables() ).includes( MapAssert.entry( "JAVA_HOME", "/some/path" ) );
}
+ /**
+ * Ensures that the environmentVariables map for launching a test jvm
+ * contains a jvm parameter-driven entry when jvm is set.
+ */
+ @Test
+ public void shouldChangeJavaHomeFromJvm() throws Exception
+ {
+ AbstractSurefireMojoTest.Mojo mojo = new AbstractSurefireMojoTest.Mojo();
+ String fakeJava = "/some/path/to/jdk/bin/java";
+ mojo.setJvm( fakeJava );
+
+ //Set up a mocked File-object
+ File mockedFile = mock( File.class );
+ when( mockedFile.getAbsoluteFile() ).thenReturn( mockedFile );
+ when( mockedFile.getPath() ).thenReturn( fakeJava );
+ when( mockedFile.getName() ).thenReturn( "java" );
+ when( mockedFile.isFile() ).thenReturn( true );
+
+ //Trap constructor calls to return the mocked File-object
+ whenNew( File.class ).
+ withParameterTypes( String.class ).
+ withArguments( anyString() ).
+ thenReturn( mockedFile );
+
+ assertThat( mojo.getEnvironmentVariables() ).isEmpty();
+ invokeMethod( mojo, "getEffectiveJvm" );
+ assertThat( mojo.getEnvironmentVariables() ).includes( MapAssert.entry( "JAVA_HOME", "/some/path/to/jdk" ) );
Review comment:
That makes sense, I never think about Windows :)
----------------------------------------------------------------
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