RFR(S): 8235454: [TESTBUG] Basic test for JFR event streaming for jdk_jfr_sanity

[mikhailo.seledtsov at oracle.com]

Hi Erik, Igor,

Pasting Igor's comment as well:

> the only problem I have w/ the test is its failure mode, other than by crash, the test can fail only by timeout, which isn't the nicest way to fail. I understand that there is no specific guarantee when an event is received by consumer, nor there are such 
> guarantees for on-close, but I'd really like us to find a better way to detect and report failures. for example, you can replace no-arg CountDownLatch::await calls w/ one which takes timeout value (await(long, j.u.c.TimeUnit)), use some reasonable timeout value > and report test failure if the return value isn't true.



IMO, whether to use explicit timeout or not in await() in the test depends on the goal of a particular test case. If the goal is to test the timing/responsiveness of the API, we should use a timeout, given that there is an explicit or implicit guarantee for event arrival (specifically that the onEvent() and onClose() should arrive within some specific time). If the goal is to simply test that events arrive, we should not use the timeout.

In this case, this is a basic/sanity test, and the goal is not to test specific timing. Hence, I am in favor of leaving the test as is, and let the test harness signal the timeout. The user will see what happened from post-test diagnostics provided. I can slightly improve the test by adding a printout statement after each await(), to let user know whether the await() is through.

Let me know what you think.


Thanks,
Misha

On 12/9/19 8:41 PM, Erik Gahlin wrote:
> Looks good
>
> When it comes to failure mode.
>
> I think it is better to let the test harness determine what is a reasonable timeout, and if needed, it can be change from the outside for all tests. Also, if the test fails using a timeout, the test harness will (hopefully) take a thread dump, which is useful for troubleshooting.
>
> Erik
>
>> On 10 Dec 2019, at 02:36, mikhailo.seledtsov at oracle.com wrote:
>>
>> Please review this change that introduces a very simple basic/sanity test for JFR streaming, and adds this test to jdk_jfr_sanity (which runs in tier-1).
>>
>>      JBS: https://bugs.openjdk.java.net/browse/JDK-8235454
>>      Webrev: http://cr.openjdk.java.net/~mseledtsov/8235454.00/
>>      Testing:
>>          1. Ran this test over 200 times on multiple platforms in the test cluster: ALL PASS
>>
>>           2. Ran the group jdk_jfr_sanity 80 times: ALL PASS
>>
>> Thank you,
>> Misha
>>