RFR(M): 8221711: [TESTBUG] create more tests for JFR in container environment

[mikhailo.seledtsov at oracle.com]

Hi Erik,

   Here is the updated webrev based on your feedback:

http://cr.openjdk.java.net/~mseledtsov/8221711.02/


Misha


On 4/12/19 7:58 AM, mikhailo.seledtsov at oracle.com wrote:
> Hi Erik,
>
>   Thank you for taking a look at the changes. Please see my comments 
> inline.
>
>
> On 4/12/19 6:48 AM, Erik Gahlin wrote:
>> Hi Misha,
>>
>> I'm a bit confused. I thought the purpose of JFrReporter was to write 
>> out the result of a recording in a container and let TestJFREvents do 
>> the actual verification by looking at the result from standard out.
>>
>> This doesn't seem to be the case with testEnvironmentVariables. It 
>> seems like it is doing both, or am I missing something?
> Yes, you are right. I deviated from the original pattern, perhaps for 
> no good reason.
>>
>> Would it be possible to simplify the JFrReporter into something like 
>> this:
>>
>> public class JfrReporter {
>>
>>   public static void main(String[] args) throws Exception {
>>     String eventName = args[1];
>>     try(Recording r = new Recording()) {
>>       r.enable(eventName);
>>       r.start();
>>       r.stop();
>>       Path p = Paths.get("tmp", eventName + ".jfr");
>>       r.dump(p);
>>       for (RecordedEvent e : RecordingFile.readAllEvents(p)) {
>>         for (ValueDescriptor v : e.getEventType().getFields()) {
>>           System.out.println(v.getName() + " = " + 
>> e.getValue(v.getName()));
>>         }
>>       }
>>     }
>>   }
>> }
>>
>> and move all the logic into TestJFREvents? Maybe there is an argument 
>> against this, but I'm curious.
> I can do that. Most likely it will work, with minor additions.
> For instance, I would like to validate some prerequisite conditions 
> (or test assumptions), such as that environment variables are defined 
> inside a container:
>
> assertTrue(System.getenv(testEnvVar) == null, "Testcase 
> TEST_ENVIRONMENT_VARS failed:" +
> 71 testEnvVar + " should not be defined inside a container");
> 72 assertTrue(System.getenv("JAVA_HOME") != null,
> 73 "Testcase ENVIRONMENT_VARS failed: JAVA_HOME should be defined 
> inside a container");
>
>  If for some reason these assumptions are wrong, it is most likely due 
> to test bug or bug in other parts of JDK, and helps us isolate the 
> root cause.
>
> Other than that, your proposal for simplification makes sense. I will 
> implement it.
>> In JfrNetwork.java, you wait for 5 seconds for a join. It seems 
>> brittle to depend on a thread getting scheduled or not. Could we 
>> create something more deterministic, and not depend on something 
>> happening within a specific time? 
> OK, this makes sense. I will remove the wait time. The join should 
> happen always; and if it does not, it indicates the error, which will 
> manifest the test timeout, and be followed by investigation. The only 
> reason IMO to use timed wait/join in such cases is to guard against 
> external/environmental circumstances that we have no control over. 
> E.g. we are waiting for another entity to accept a connection, I/O 
> that may take longer time due to system or HW factors, etc. In this 
> particular case looks like our code is in control, so it should be 
> fine to remove the timed wait.
>
>
> I will make the changes, and post an updated webrev.
>
> Thank you,
> Misha
>> Thanks
>> Erik
>>
>>>
>>>
>>> On 4/8/19 8:28 PM, mikhailo.seledtsov at oracle.com wrote:
>>>> Please review this change that adds two more test cases for JFR 
>>>> events in the container environment:
>>>>   - making sure environment variables are reported correctly inside 
>>>> the container
>>>>   - host info is reported correctly inside the container
>>>>
>>>>     JBS: https://bugs.openjdk.java.net/browse/JDK-8221711
>>>>     Webrev: http://cr.openjdk.java.net/~mseledtsov/8221711.01/
>>>>     Testing:
>>>>       Ran the affected tests on a single host - PASS
>>>>       On multiple host variety - In PROGRESS
>>>>
>>>>
>>>> Thank you,
>>>> Misha
>>>>
>>>
>>
>