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

[Erik Gahlin]

Looks good.

You can remove runTest method and no need with parentheses around (new 
TestEvent()).commit()

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.

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