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:01:14 UTC 2016
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