RFR(M): JDK-8165805 - [TESTBUG] CDS needs to support JVMTI CFLH - test development
Mikhailo Seledtsov
mikhailo.seledtsov at oracle.com
Thu Sep 15 04:04:58 UTC 2016
Thank you,
Misha
On 9/14/16, 9:01 PM, serguei.spitsyn at oracle.com wrote:
> Hi Misha,
>
>
> On 9/14/16 18:17, Mikhailo Seledtsov 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
>> - some other method I do not know about - suggestions welcome
>>
>> Let me know which one you'd like me to use.
>
> Ok, let's keep it as it is.
> My suggestion is for a minor improvement, not worth to add any kind of
> complexity.
>
>
>
>>> 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.
>
> Ok.
>
>
>>>
>>> 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.
>
>
> Sorry, I got it wrong.
> Would it be better to use angle brackets and commas for fields separation?
>
>
>>>
>>> 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.
>
>
> I just see no need for a table here.
> But no pressure - it is up to you.
>
>
>>
>>
>>
>> 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.
>
> All is set now. :)
>
> Thanks!
> Serguei
>
>
>>
>> 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