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

[mikhailo.seledtsov at oracle.com]

Here is the updated webrev: 
http://cr.openjdk.java.net/~mseledtsov/8222888.01/index.html


Thank you,

Misha


On 4/25/19 8:27 AM, mikhailo.seledtsov at oracle.com wrote:
> 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
>>
>