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

[Mikhailo Seledtsov]

Hi Serguei,

   Thank you for review. Please see my comments inline.


On 9/12/16, 7:03 PM, serguei.spitsyn at oracle.com wrote:
> Hi Misha,
>
> The tests are nicely organized.
>
> Just minor comments now:
>
> The patterns must be defined only once as static final strings:
>    static final String BeforePattern = "this-should-be-transformed ";
>    static final String AfterPattern  = "this-has-been--transformed";
>
> Not sure yet, where to define it better.
> It seems, the TransformUtil class can be a good place.
Good point. I have defined the constants in TransformUtil, and updated 
relevant places to use these constants.
> Then the 'from' and 'to' parameters in the TransformUtil.replace() can 
> be deleted.
> It is if it is not important to keep this method universal.
>
I would like to keep it universal.
>
> http://cr.openjdk.java.net/~mseledtsov/8165805.00/test/runtime/SharedArchiveFile/serviceability/TransformUtil.java.html 
>
>
>   21         for (int i=0; i<max; ) {
>   33         for (int x=0; x<f.length; x++) {
>   38         for (int x=0; x<f.length; x++) {
>   46         for (int i=0; i<b.length; i++) {
>
>   Need spaces around the '=' and '<' signs.
>
>   24                 numReplaced ++;
>
>   Unneeded space.
>
Fixed here, and in all other for loops in the file.
>
> http://cr.openjdk.java.net/~mseledtsov/8165805.00/test/runtime/SharedArchiveFile/serviceability/TransformerAgent.java.html 
>
>
>   12 // This is a test ulitilty class used to transform
>   13 // specified classes via initial transformation (CFLH)
>   14 // Names of classes to be transformed are supplied as arguments,
>
>   Need a dot at the end of L13.
>
>
Fixed.
> http://cr.openjdk.java.net/~mseledtsov/8165805.00/test/runtime/SharedArchiveFile/serviceability/transformRelatedClasses/TransformTestCommon.java.html 
>
>
>   24         if(entry.transformParent && entry.transformChild)
>   26         if(entry.transformParent)
>   28         if(entry.transformChild)
>   53         if(entry.isParentExpectedShared)
>   58         if(entry.isChildExpectedShared)
>
>   Space is needed after 'if'.
Fixed.
>
>   71         out.shouldContain("TransformerAgent: SimpleTransformer 
> called for: " + child + "@1");
>   72         out.shouldContain("TransformerAgent: SimpleTransformer 
> called for: " + parent + "@1");
>   73
>   74         out.shouldNotContain("TransformerAgent: SimpleTransformer 
> called for: " + child + "@2");
>   75         out.shouldNotContain("TransformerAgent: SimpleTransformer 
> called for: " + parent + "@2");
>
>   Can be simplified:
>          String patternBase = "TransformerAgent: SimpleTransformer 
> called for: ";
>          out.shouldContain(patternBase + child + "@1");
>          out.shouldContain(patternBase + parent + "@1");
>          out.shouldNotContain(patternBase + child + "@2");
>          out.shouldNotContain(patternBase + parent + "@2");
>
Fixed.
>
>   92         checkSharingByClass(entry,out, parent, child);
>
>   Space is missed before 'out'.
>
Fixed.

I have addressed all the review comments from you, Jiangli and Ioi. I 
will test the tests, and post the updated webrev shortly.

Thank you,
Misha
>
> Thanks,
> Serguei
>
>
>
> On 9/9/16 19:31, 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
>>
>