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