RFR(M): JDK-8165805 - [TESTBUG] CDS needs to support JVMTI CFLH - test development
Mikhailo Seledtsov
mikhailo.seledtsov at oracle.com
Thu Sep 15 04:00:28 UTC 2016
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