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

[mikhailo.seledtsov at oracle.com]

Hi Severin,

   Good catch. What I really meant to do is this:

     pb.environment().put(TEST_ENV_VARIABLE, TEST_ENV_VALUE);
         OutputAnalyzer out = new OutputAnalyzer(pb.start())  // <<==== 
start the process for new process builder with environment var defined
             .shouldHaveExitValue(0)
             .shouldContain("key = JAVA_HOME")
             .shouldContain("value = /jdk")

It was an unfortunate copy/paste error, thanks for catching it.

I will run tests, and update a webrev shortly.


Thank you,
Misha


On 4/25/19 1:17 AM, Severin Gehwolf wrote:
> 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
>