RFR(S): 8213914: [TESTBUG] Several JFR VM events are not covered by tests

[Mikhailo Seledtsov]

Hi Erik, Markus,

   I have updated the webrev again: 
http://cr.openjdk.java.net/~mseledtsov/8213914.02/

Mainly, I incorporated searching for references to JFR event names from 
JFR tests (what I earlier accomplished via the bash script, now in 
Java/JTReg).

Thank you,
Misha

On 11/15/18, 8:19 PM, Mikhailo Seledtsov wrote:
> Hi Erik, Markus,
>
>   Thank you for the first round of reviews. Here is a webrev that 
> incorporates updates based on your feedback:
> http://cr.openjdk.java.net/~mseledtsov/8213914.01/
>
> Please review.
>
> Thank you,
> Misha
>
> On 11/15/18, 5:03 PM, Mikhailo Seledtsov wrote:
>> Hi Erik,
>>
>>   Thank you for this recommendation. I have wrote a test 
>> "TestLookForUntestedEvents.java" and placed it under 
>> open/test/jdk/jdk/jfr/event/metadata/
>> Please let me know if you have different suggestions for the test 
>> name or directory placement. The preliminary content of the test is 
>> attached.
>>
>> Originally, I added the message to the exception, as you outlined in 
>> your email. But it did not look good, so I printed it to stdout 
>> instead, with some formatting, for easy detection and readability. 
>> Let me know if you wish me to change it.
>>
>> Currently, the test fails with the following message:
>> =========
>> Set jfrEventTypes has more elements than set eventsFromEventNamesClass:
>> ReservedStackActivation
>> ParallelOldGarbageCollection
>> ZStatisticsSampler
>> ZStatisticsCounter
>> ZPageAllocation
>> ZThreadPhase
>>
>> This could be because:...
>> ==========
>>
>> As I understand, it would be good to include this test with this fix. 
>> In such case, I will file bugs on the above mentioned sets, and will 
>> add an "Known failure set" into this test, with appropriate bug 
>> numbers and comments.
>> Let me know if this works for you.
>>
>> Thank you,
>> Misha
>>
>>
>> On 11/15/18, 10:24 AM, Erik Gahlin wrote:
>>> Hi Misha,
>>>
>>> Would it be possible to create a metatest to make sure JFR events 
>>> are always tested? Something like this:
>>>
>>> /**
>>>  * @test
>>>  * @key jfr
>>>  * @summary Make sure all events have a unit test
>>>  * @requires vm.hasJFR
>>>  * @library /test/lib
>>>  * @run jdk.jfr.TestTests
>>>  */
>>> public class TestTests {
>>>
>>>     public static void main(String... args) throws Exception {
>>>         Set<String> eventsInTests = new HashSet<>();
>>>         for (Field f : EventNames.class.getFields()) {
>>>             String name = f.getName();
>>>             if (!name.equals("PREFIX")) {
>>>                 eventsInTests.add(name);
>>>             }
>>>         }
>>>         for (EventType type : 
>>> FlightRecorder.getFlightRecorder().getEventTypes()) {
>>>             if (type.getAnnotation(Experimental.class) == null) {
>>>                 if (!eventsInTests.contains(type.getName())) {
>>>                     String message = "Could not find unit test for 
>>> event " + type.getName() + "\n";
>>>                     message += "This could be because:\n";
>>>                     message += "1) You forgot to write a unit test. 
>>> Please do so in test/jdk/jdk/jfr/event/ \n";
>>>                     message += "2) You wrote a unit test, but you 
>>> didn't reference it in test/lib/jdk/test/lib/jfr/EventNames.java.\n";
>>>                     message += "3) It is not feasible to test the 
>>> event, not even a sanity test. Add the event name to 
>>> test/lib/jdk/test/lib/jfr/EventNames.java and a short comment why it 
>>> can't be tested\n";
>>>                     message += "4) The event is experimental. Please 
>>> add 'experimental=\"true\"' to <Event> element in metadata.xml if it 
>>> is a native event, or @Experimental if it is a Java event. The event 
>>> will now not show up in JMC\n.";
>>>                     throw new Exception(message);
>>>                 }
>>>             }
>>>         }
>>>     }
>>> }
>>>
>>> The GC category check could be changed to:
>>>
>>>  public static boolean isGcEvent(EventType et) {
>>>         return et.getCategoryNames().contains("GC");
>>>  }
>>>
>>> Thanks
>>> Erik
>>>
>>>> Hi Markus,
>>>>
>>>>   I wondered this myself. But only one event actually used the 
>>>> wrong name (EvacuationInfo), and the test passed. Here is a 
>>>> detailed breakdown on the change:
>>>>
>>>> This change contains fixes that can be divided into 3 categories:
>>>>   - the events that were not listed in 
>>>> test/lib/.../EventNames.java, hence were not tested at all: " 
>>>> GCPhasePauseLevel4", " GCPhaseConcurrent"
>>>>   - events were spelled correctly, but their references/aliases 
>>>> used shorter names:
>>>>     EventNames. CompilerConfig --> EventNames.CompilerConfiguration,
>>>>     EventNames.SafepointStateSyncronization --> 
>>>> EventNames.SafepointStateSynchronization  (syncr --> synchr)
>>>>     This part of the change is to satisfy my verification scripts 
>>>> that reads event names from metadata.xml, and then verifies that 
>>>> the names are used/mentioned in the jfr tests.
>>>>   - only one case where the wrong event name was used, and the test 
>>>> actually passed: EvacuationInfo --> EvacuationInformation
>>>>      Not sure why test passed before. I can do a quick check now 
>>>> how this happened, and possibly file an RFE if this leads to a 
>>>> longer investigation.
>>>>
>>>> Thank you,
>>>> Misha
>>>>
>>>> On 11/15/18, 1:35 AM, Markus Gronlund wrote:
>>>>> Hi Misha,
>>>>>
>>>>> Thanks for looking into this.
>>>>>
>>>>> What exactly are the situation being addressed here?
>>>>> Have we been running tests with the wrong Event names in addition to the tests not being able to detect this? That is, the tests have been run, reported successful, but not really testing anything?
>>>>>
>>>>> Markus
>>>>>
>>>>> -----Original Message-----
>>>>> From: Mikhailo Seledtsov
>>>>> Sent: den 15 november 2018 05:02
>>>>> To:hotspot-jfr-dev at openjdk.java.net; Erik Gahlin<erik.gahlin at oracle.com>; Markus Gronlund<markus.gronlund at oracle.com>
>>>>> Subject: RFR(S): 8213914: [TESTBUG] Several JFR VM events are not covered by tests
>>>>>
>>>>> Please review this fairly small change that enables/adds testing for several JVM JFR events that were not covered.
>>>>>       JBS:https://bugs.openjdk.java.net/browse/JDK-8213914
>>>>>       Webrev:http://cr.openjdk.java.net/~mseledtsov/8213914.00/index.html
>>>>>       Testing:
>>>>>           Ran open/test/jdk/jdk/jfr/ on multiple platforms
>>>>>           All PASS
>>>>>
>>>>>
>>>>> Thank you,
>>>>> Misha
>>>>>
>>>