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

[serguei.spitsyn at oracle.com]

Hi Misha,


Looks good in general.


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.


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");


    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");


    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");


    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.



   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));


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
>>