RFR(M): JDK-8165805 - [TESTBUG] CDS needs to support JVMTI CFLH - test development

serguei.spitsyn at oracle.com serguei.spitsyn at oracle.com
Thu Sep 15 04:12:27 UTC 2016


Misha,

http://cr.openjdk.java.net/~mseledtsov/8165805.03/test/runtime/SharedArchiveFile/serviceability/transformRelatedClasses/TransformRelatedClasses.java.html

   35 // Here are the details on the structure of the test

   Minor: Dot at the end is missed.


It looks good.


Thanks,
Serguei


On 9/14/16 21:00, Mikhailo Seledtsov wrote:
> Jiangli, Serguei,
>
>  Thank you for answers on the last renaming question.
>
> Here is an updated webrev that implements feedback for the second 
> round of review:
> http://cr.openjdk.java.net/~mseledtsov/8165805.03/
>
> Thank you,
> Misha
>
> On 9/14/16, 8:47 PM, serguei.spitsyn at oracle.com wrote:
>> On 9/14/16 20:45, Jiangli Zhou wrote:
>>> Hi Misha,
>>>
>>>> On Sep 14, 2016, at 6:17 PM, Mikhailo Seledtsov 
>>>> <mikhailo.seledtsov at oracle.com> wrote:
>>>>
>>>> Hi Serguei,
>>>>
>>>> Thank you for second round of review. See my comments inline:
>>>>
>>>> On 9/14/16, 3:28 PM, serguei.spitsyn at oracle.com wrote:
>>>>> Hi Misha,
>>>>>
>>>>>
>>>>> Looks good in general.
>>>>>
>>>> Thank you.
>>>>> Few comments.
>>>>>
>>>>> http://cr.openjdk.java.net/~mseledtsov/8165805.02/test/runtime/SharedArchiveFile/serviceability/transformRelatedClasses/TransformInterfaceAndImplementor.java.html 
>>>>>
>>>>> http://cr.openjdk.java.net/~mseledtsov/8165805.02/test/runtime/SharedArchiveFile/serviceability/transformRelatedClasses/TransformSuperAndSubClasses.java.html 
>>>>>
>>>>> http://cr.openjdk.java.net/~mseledtsov/8165805.02/test/runtime/SharedArchiveFile/serviceability/transformRelatedClasses/TransformSuperSubTwoPckgs.java.html 
>>>>>
>>>>>
>>>>>   Perhaps, the java.instrument must be added to the list of @modules.
>>>>>
>>>> OK, done.
>>>>> http://cr.openjdk.java.net/~mseledtsov/8165805.02/test/runtime/SharedArchiveFile/serviceability/transformRelatedClasses/myPkg1/SuperClazz.java.html 
>>>>>
>>>>>
>>>>>   13         System.out.println("parent-transform-check: 
>>>>> this-should-be-transformed");
>>>>>
>>>>>
>>>> This actually is a problem. Nothing that can not be solved, but as 
>>>> far as I know no simple solution for it.
>>>>
>>>> In summary, as far as I understand (and searched for it some, but 
>>>> correct me please if I am wrong): " It is a compile time error to 
>>>> import a type from the unnamed package."
>>>>
>>>> In detail:
>>>>   - traditionally all CDS tests (as many other hotpost JT-Reg 
>>>> tests) use default package. Not the best way, but it is what it is 
>>>> now.
>>>>   - the constants for "parent-transform-check" etc are defined in 
>>>> TransformUtil, which is assigned no package (hence it belongs to 
>>>> "default", or "unnamed" package).
>>>>   - most of tests using these constants also reside in "unnamed" 
>>>> package, so it works
>>>>   - however, this specific test requires for archived test classes 
>>>> to be in packages, as part of test scenario
>>>>   - AFAIK, it is not possible to import types from unnamed package 
>>>> into a named package
>>>>   - hence, I can not reference TransformUtil.ParentCheckPattern 
>>>> (and other pattern constants) from myPkg1.SuperClazz
>>>>
>>>> Well, given that, I can think of several solutions. Please let me 
>>>> know which one you prefer:
>>>>   - place TransformUtil in a package. Not sure how this will work 
>>>> out with all the tests dependent on this utility, I foresee some 
>>>> complications.
>>>>      Besides, there is a standing future plan to put JT-Reg tests 
>>>> in packages, which will have discussions on naming etc. Of course, 
>>>> I can pick a name for now, which we will end up changing later.
>>>>
>>>>   - use reflection, something like:
>>>>      String parentCheckPattern = 
>>>> Class.forName("TransformUtil").getField(ParentCheckPattern)...
>>>>      Seems bit to complex for this case
>>>>   - leave it as is, and add a comment of where the constant is 
>>>> defined, and some explanation why it can not be accessed
>>> It doesn't seem worth using reflection in this case. I vote for 
>>> leaving it as is.
>>
>> Agreed.
>> It is better to leave it as it is - even no comments.
>> I'm against adding any kind of complexity.
>>
>> Thanks,
>> Serguei
>>
>>
>>>
>>> Thanks,
>>> Jiangli
>>>
>>>>   - some other method I do not know about - suggestions welcome
>>>>
>>>> Let me know which one you'd like me to use.
>>>>>    Need to use the ParentCheckPattern and BeforePattern.
>>>>>
>>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~mseledtsov/8165805.02/test/runtime/SharedArchiveFile/serviceability/transformRelatedClasses/myPkg2/SubClass.java.html 
>>>>>
>>>>>
>>>>>   18         System.out.println("child-transform-check: 
>>>>> this-should-be-transformed");
>>>>>
>>>> Same as above.
>>>>>    Need to use the ChildCheckPattern and BeforePattern.
>>>>>
>>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~mseledtsov/8165805.02/test/testlibrary/jvmti/TransformerAgent.java.html 
>>>>>
>>>>>
>>>>>   68             int nrOfReplacements = 
>>>>> TransformUtil.replace(buffer, "this-should-be-transformed", 
>>>>> "this-has-been--transformed");
>>>>>
>>>> I can fix this.
>>>>>    Need to use the BeforePattern and AfterPattern.
>>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~mseledtsov/8165805.02/test/runtime/SharedArchiveFile/serviceability/transformRelatedClasses/TransformRelatedClasses.java.html 
>>>>>
>>>>>
>>>>>   96         // Test Table
>>>>>   97         // testCaseId |  transformParent | tranformChild | 
>>>>> isParentExpectedShared | isChildExpectedShared
>>>>>
>>>>>
>>>>>     For clarity, the comment above should list the test cases in 
>>>>> the same order as they are executed below.
>>>>>     It seems, the test cases listed in the comment above do not 
>>>>> match the test cases below.
>>>>>     I see just 4 test cases below, not 5 as listed above. Perhaps, 
>>>>> the testCaseId above is not a test case?
>>>>>     Also, the base case below is not listed in the comment above.
>>>>>
>>>>>
>>>> Sorry if the comment is a bit confusing. The keywords delimited by 
>>>> the vertical line are supposed to list the columns of the test 
>>>> table. Kind of like a table header.
>>>> Not the list of the test cases.
>>>>
>>>> If you find it too confusing, please let me know - I will delete 
>>>> the comment altogether.
>>>>>   98 ArrayList<TestEntry> testTable = new ArrayList<>();
>>>>>   99
>>>>> 100         // base case - no tranformation - all expected to be 
>>>>> shared
>>>>> 101         testTable.add(new TestEntry(0, false, false, true, 
>>>>> true));
>>>>> 102
>>>>> 103         // transform parent only - both parent and child 
>>>>> should not be shared
>>>>> 104         testTable.add(new TestEntry(1, true, false, false, 
>>>>> false));
>>>>> 105
>>>>> 106         // transform parent and child - both parent and child 
>>>>> should not be shared
>>>>> 107         testTable.add(new TestEntry(2, true, true, false, 
>>>>> false));
>>>>> 108
>>>>> 109         // transform child only - parent should still be 
>>>>> shared, but not child
>>>>> 110         testTable.add(new TestEntry(3, false, true, true, 
>>>>> false));
>>>>> 111
>>>>> 112         // run the tests
>>>>> 113         for (TestEntry entry : testTable) {
>>>>> 114             test.runTest(entry);
>>>>> 115         }
>>>>>
>>>>>    Nit:
>>>>>      Not sure why the testTable is needed as it could be 
>>>>> simplified as below:
>>>>>
>>>>>           // base case - no tranformation - all expected to be shared
>>>>>           test.runTest(new TestEntry(0, false, false, true, true));
>>>>>             // transform parent only - both parent and child 
>>>>> should not be shared
>>>>>           test.runTest(new TestEntry(1, true, false, false, false));
>>>>>             // transform parent and child - both parent and child 
>>>>> should not be shared
>>>>>           test.runTest(new TestEntry(2, true, true, false, false));
>>>>>             // transform child only - parent should still be 
>>>>> shared, but not child
>>>>>           test.runTest(new TestEntry(3, false, true, true, false));
>>>>>
>>>> This is a pattern of data-driven testing. Just a matter of style in 
>>>> my opinion. The pattern is: fill out the table first, and then 
>>>> execute the tests based on the table.
>>>> It is not strictly necessary here, but there are some cases where 
>>>> tables are much bigger, and can be passed from somewhere else 
>>>> ("test definer") to the "test executor".
>>>> I would prefer to keep it, unless you have strong objection on 
>>>> this. We may expand these test cases in the future.
>>>>
>>>>
>>>>
>>>> Let me know what is your opinion on my questions/comments above. 
>>>> Once I have the answers, I will make the updates and post next webrev.
>>>>
>>>> Thank you,
>>>> Misha
>>>>> Thanks,
>>>>> Serguei
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> On 9/13/16 22:08, Mikhailo Seledtsov wrote:
>>>>>> Thank you again for review. I have implemented all the review 
>>>>>> recommendations.
>>>>>> The update also contains new test case from Jiangli to test 
>>>>>> super/sub class pair, where super and sub classes belong to 
>>>>>> different packages.
>>>>>> Thank you Jiangli for the test case.
>>>>>>
>>>>>> Here is an updated webrev: 
>>>>>> http://cr.openjdk.java.net/~mseledtsov/8165805.02/
>>>>>>
>>>>>> Thank you,
>>>>>> Misha
>>>>>>
>>>>>> On 9/9/16, 7:31 PM, Mikhailo Seledtsov wrote:
>>>>>>>     Please review the tests that accompany the changes for
>>>>>>>     "JDK-8078644 - CDS needs to support JVMTI CFLH"
>>>>>>>
>>>>>>>     The tests include several test cases that are produced by 
>>>>>>> combination
>>>>>>>     of transforming related classes: super/sub class and 
>>>>>>> interface/implementor.
>>>>>>>     The classes reside in CDS archive. Tests check to ensure 
>>>>>>> that correct
>>>>>>>     transformation is performed, classes are loaded from the 
>>>>>>> archive only when appropriate,
>>>>>>>     and that test process using the archive completes w/o 
>>>>>>> errors/crashes.
>>>>>>>
>>>>>>>
>>>>>>>     Details:
>>>>>>>
>>>>>>>     The main class is TransformRelatedClasses.java; start review 
>>>>>>> with this class.
>>>>>>>     Other classes are:
>>>>>>>       - TransformSuperAndSubClasses.java, 
>>>>>>> TransformInterfaceAndImplementor.java
>>>>>>>         the "actual tests", but really just test headers calling 
>>>>>>> into TransformRelatedClasses.java
>>>>>>>
>>>>>>>       - TransformTestCommon.java - common methods for 
>>>>>>> transformation tests
>>>>>>>
>>>>>>>       - TestEntry.java - an entry into combinatorial test table 
>>>>>>> representing a single test case
>>>>>>>
>>>>>>>       - CDSTestUtils.java, TransformUtil.java - utility classes 
>>>>>>> that are common  CDS
>>>>>>>         tests
>>>>>>>
>>>>>>>       - TransformerAgent.java - agent that performs 
>>>>>>> transformation (comes with .mf file)
>>>>>>>
>>>>>>>
>>>>>>>     JBS: https://bugs.openjdk.java.net/browse/JDK-8165805
>>>>>>>     Webrev: http://cr.openjdk.java.net/~mseledtsov/8165805.00/
>>>>>>>     Testing:
>>>>>>>         Ran these tests on a standard set of platforms thru 
>>>>>>> automated test system.
>>>>>>>
>>>>>>> Thank you,
>>>>>>> Misha
>>>>>>>
>>



More information about the hotspot-runtime-dev mailing list