RFR(S): 8222888: [TESTBUG] docker/TestJFREvents.java fails due to "RuntimeException: JAVA_MAIN_CLASS_* is not defined"

[Severin Gehwolf]

Hi Misha,

Thanks for fixing this!

On Wed, 2019-04-24 at 13:06 -0700, mikhailo.seledtsov at oracle.com wrote:
> Please review this change that makes a test more robust. The test 
> originally relied on the fact that JAVA_MAIN_CLASS variable
> is always present when running jtreg tests. This assumption is wrong, 
> hence I reworked the test to define its own unique
> environment variable.
> 
> In order to introduce environment variable I reworked the 
> DockerTestUtils a little bit, by splitting the
> dockerRunJava() method into 2: buildJavaCommand() to build the command; 
> dockerRunJava() uses buildJavaCommand()
> then runs the command. This allowed me to introduce the test environment 
> variable to the child process.
> The behavior of dockerRunJava() should not be affected by this change, 
> it is a simle split.
> 
>      JBS: https://bugs.openjdk.java.net/browse/JDK-8222888
>      Webrev: http://cr.openjdk.java.net/~mseledtsov/8222888.00/
>      Testing:
>          1. Ran hotspot docker tests on Linux-x64 machine with docker 
> enigne configured.
>             Ran both via jtreg directly and via make
>             All PASS

This doesn't seem right to me:

     private static void testEnvironmentVariables() throws Exception {
         Common.logNewTestCase("EnvironmentVariables");
 
-        DockerTestUtils.dockerRunJava(
+        List<String> cmd = DockerTestUtils.buildJavaCommand(
                                       commonDockerOpts()
-                                      .addClassOptions("jdk.InitialEnvironmentVariable"))
+                                      .addClassOptions("jdk.InitialEnvironmentVariable"));
+
+        ProcessBuilder pb = new ProcessBuilder(cmd);
+
+        // Container has JAVA_HOME defined via the Dockerfile; make sure
+        // it is reported by JFR event.
+        // Environment variable set in host system should not be visible inside a container,
+        // and should not be reported by JFR.
+        pb.environment().put(TEST_ENV_VARIABLE, TEST_ENV_VALUE);
+        DockerTestUtils.execute(cmd)
             .shouldHaveExitValue(0)
             .shouldContain("key = JAVA_HOME")
-            .shouldNotContain(getTestEnvironmentVariable());
+            .shouldContain("value = /jdk")
+            .shouldNotContain(TEST_ENV_VARIABLE)
+            .shouldNotContain(TEST_ENV_VALUE);

So you are creating a new ProcessBuilder instance, add the environment
variable, but then never start it? This means that not even the docker
run cmd will have the environment. Let alone the JVM inside docker.

Note that DockerTestUtils.execute() has it's own version of
ProcessBuilder. If I'm reading this right, that should be the function
that needs to conditionally add the environment.

Am I missing something?

Thanks,
Severin