RFR(S): 8167493: Test that JFR event can be retransformed by an agent

[mikhailo.seledtsov at oracle.com]

Erik,

Thank you for Review. I have made the updates that you have recommended 
prior to the push, and pushed.

On 2/14/20 11:08 AM, Erik Gahlin wrote:
> Looks good.
>
> You can remove runTest method and no need with parentheses around (new 
> TestEvent()).commit()
Done
>
> I wonder if it would be helpful for test writers if we also add these 
> attributes by default:
>
> attrs.putValue("Can-Redefine-Classes", "true");
> attrs.putValue("Can-Retransform-Classes", "true");
>
> Most tests that use JavaAgentBuilder are likely to want those turned 
> on. You can keep the existing parse logic so it possible to add 
> additional attributes or override the default ones.
Sounds good to me.
>
> No need to send out updated review.
>
> Thanks
> Erik
>
>
> On 2020-02-14 19:03, mikhailo.seledtsov at oracle.com wrote:
>> Here is the updated webrev: 
>> http://cr.openjdk.java.net/~mseledtsov/8167493.03/
>>
>> On 2/14/20 9:40 AM, Erik Gahlin wrote:
>>> Yes, that sums it up well.
>>>
>>> I think it would be good to use try-with-resources with the 
>>> recording object and make the transformer a separate inner class to 
>>> make it more explicit that the transform method belongs to the 
>>> transformer.
>>>
>>> Erik
>>>
>>> On 2020-02-14 18:19, mikhailo.seledtsov at oracle.com wrote:
>>>> Hi Erik,
>>>>
>>>>   Thank you for the initial review. I looked thru your proposed 
>>>> logic, and it makes sense to me. If I understand correctly, changes 
>>>> you propose can be summarized as:
>>>>
>>>>    - simplify premain and move all test logic to main
>>>>
>>>>    - test case that tests construction of the event only, w/o commit
>>>>
>>>>    - test case with recording & commit, and assert that the event 
>>>> has been recorded (in addition to instrumentation callback)
>>>>
>>>>
>>>> I will update the test accordingly, retest and post updated webrev.
>>>>
>>>> Misha
>>>>
>>>> On 2/14/20 9:12 AM, Erik Gahlin wrote:
>>>>> Hi Misha,
>>>>>
>>>>> I think this tests things as they should but I'm not 100% sure. It 
>>>>> may make sense to change the flow a bit so it becomes more 
>>>>> explicit. if you only does this in premain.
>>>>>
>>>>> public static void premain(String args, Instrumentation inst) 
>>>>> throws Exception {
>>>>>    instrumentation = inst;
>>>>> }
>>>>>
>>>>> then in main:
>>>>>
>>>>> 1. TestEvent event = new TestEvent(); // loads class and run empty 
>>>>> constructor
>>>>> 2. instrumentation.addTransformer(new TestClassFileTransformer());
>>>>> 3. instrumentation.retransformClasses(TestEvent.class);
>>>>> 4. TestEvent event = new TestEvent(); // instrumented constructor
>>>>> 5. Asserts.assertTrue(InstrumentationEventCallback.wasCalled());
>>>>> 6. InstrumentationEventCallback.clear();
>>>>> 7. Recording r = new Recording(); r.enable(TestEvent.class); 
>>>>> r.start();
>>>>> 8. TestEvent event = new TestEvent(); event.commit(); // JFR 
>>>>> instrumented commit
>>>>> 9. Asserts.assertTrue(InstrumentationEventCallback.wasCalled());
>>>>> 10. r.dump(file)
>>>>> 11. Asserts.assertTrue(!RecordingFile.readAllEvents().isEmpty())
>>>>>
>>>>> Thanks
>>>>> Erik
>>>>>
>>>>> On 2020-02-13 17:12, mikhailo.seledtsov at oracle.com wrote:
>>>>>
>>>>>> Please review this change that adds a new test to verify that a 
>>>>>> user-defined JFR event can be tramsformed by an agent.
>>>>>> This test uses a class transformer to add a callback into the 
>>>>>> <init> of the event class, generates/records the event,
>>>>>> and verifies that the added instrumentation callback was actually 
>>>>>> called.
>>>>>>
>>>>>> Also, the JavaAgentBuilder testlib utility was extended to accept 
>>>>>> additional manifest attributes.
>>>>>>
>>>>>>     JBS: https://bugs.openjdk.java.net/browse/JDK-8167493
>>>>>>     Webrev: http://cr.openjdk.java.net/~mseledtsov/8167493.02/
>>>>>>     Testing:
>>>>>>         1. Ran the newly created test: PASS
>>>>>>         2. Ran all tests using JavaAgentBuilder: PASS
>>>>>>
>>>>>>
>>>>>> Thank you,
>>>>>> Misha
>>>>>>