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

[Daniel D. Daugherty]

On 9/13/16 11:08 PM, 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/

test/runtime/SharedArchiveFile/CDSTestUtils.java
     L50:         if ( (output.getExitValue() == 1) && (
         nit: extrace space between '(' and '('.

         Would an exit value != 0 be better/more general? I'm going to
         guess that all of the mapping failure strings are accompanied
         by an exit_code == 1.

     L61:         } else {
     L62:             return false;
     L63:         }
     L64:     }

         Perhaps:

         L61:         }
         L62:         return false;
         L63:     }

     L66:     // check result of 'exec' operation, that is when JVM is 
ran using the archive
         Typo: 'is ran' -> 'is run'



test/runtime/SharedArchiveFile/serviceability/transformRelatedClasses/Implementor.java
     No comments.

test/runtime/SharedArchiveFile/serviceability/transformRelatedClasses/Interface.java
     No comments.

test/runtime/SharedArchiveFile/serviceability/transformRelatedClasses/SubClass.java
     No comments.

test/runtime/SharedArchiveFile/serviceability/transformRelatedClasses/SuperClazz.java
     No comments.

test/runtime/SharedArchiveFile/serviceability/transformRelatedClasses/TestEntry.java
     No comments.

test/runtime/SharedArchiveFile/serviceability/transformRelatedClasses/TransformInterfaceAndImplementor.java
     L26  * @summary Exercise initial transformation (class file loader 
hook)

         Typo: "loader" -> "load" or use the event name: ClassFileLoadHook

     L29:  * @requires (vm.opt.UseCompressedOops == null) | 
(vm.opt.UseCompressedOops == true)
     L30:  * @requires vm.flavor != "minimal"
         Should there be a comment somewhere explaining why this test
         can't be run with -XX:-UseCompressedOops or with the minimal VM?
         I'm not completely up to speed to @requires style...

     L41: // The transformation is done via Java Instrumentaiton
         typo: 'Java Instrumentaiton' -> JLI/JPLIS

     L42: // Class File Loader Hook facility.
         typo: 'Loader' -> 'Load'

test/runtime/SharedArchiveFile/serviceability/transformRelatedClasses/TransformRelatedClasses.java
     L40: //     TransformInterfaceAndImplementor, 
TransformSuperAndSubClasses.java
         Remove '.java' from the end since you are talking about classes?

     L42: //     the shared archive), running test cases and checking 
the results.
         Typo: 'the shared' -> 'and the shared'

     L49: // For more details, see the test classes code and comments.
         Typo: "test classes" -> "test classes'" (possessive)

     L55: // Other utility/helper classes used in this test:
         Perhaps "classes" -> "classes and files" since TransformerAgent.mf
         is not a class.

     L56: //     TransformerAgent -- an agent that is used when 
JVM-under-test is executed
         Typo: '--' -> '-' (consistency)

     L59: //     CDSTestUtils.java - Test Utilities common to all CDS tests
         Remove '.java' since you are talking about classes?

test/runtime/SharedArchiveFile/serviceability/transformRelatedClasses/TransformSuperAndSubClasses.java
     L27:  * @summary Exercise initial transformation (class file loader 
hook)

         Typo: "loader" -> "load" or use the event name: ClassFileLoadHook

     Same question about @requires lines.

     L42: // The transformation is done via Java Instrumentaiton
         typo: 'Java Instrumentaiton' -> JLI/JPLIS

     L43: // Class File Loader Hook facility.
         typo: 'Loader' -> 'Load'

test/runtime/SharedArchiveFile/serviceability/transformRelatedClasses/TransformSuperSubTwoPckgs.java
     L27:  * @summary Exercise initial transformation (class file loader 
hook)

         Typo: "loader" -> "load" or use the event name: ClassFileLoadHook

     Same question about @requires lines.

     L43: // The transformation is done via Java Instrumentaiton
         typo: 'Java Instrumentaiton' -> JLI/JPLIS

     L44: // Class File Loader Hook facility.
         typo: 'Loader' -> 'Load'

test/runtime/SharedArchiveFile/serviceability/transformRelatedClasses/TransformTestCommon.java
     L33:     public static String
     L34:         getAgentParams(TestEntry entry, String parent, String 
child) {
         These two lines broken in a strange place. If the function def 
is too
         long the line break is usually in the parameter list.

     L99:         // If was not able to map an archive,
         Typo: "If was not" -> "If we were not"

test/runtime/SharedArchiveFile/serviceability/transformRelatedClasses/myPkg1/SuperClazz.java
     No comments.

test/runtime/SharedArchiveFile/serviceability/transformRelatedClasses/myPkg2/SubClass.java
     No comments.

test/testlibrary/jvmti/TransformUtil.java
     No comments.

test/testlibrary/jvmti/TransformerAgent.java
     L25: import java.lang.instrument.Instrumentation;
     L26: import java.lang.instrument.IllegalClassFormatException;
         nit: wrong sort order

     L30: // This is a test ulitilty class used to transform
         typo: 'ulitilty' -> 'utility'

test/testlibrary/jvmti/TransformerAgent.mf
     No comments.


Thumbs up! I don't see anything other than typos and nits here.
Feel free to clean these up or not. Your call.

Dan


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