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

[mikhailo.seledtsov at oracle.com]

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
>>>
>>
>