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

[Mikhailo Seledtsov]

Hi Dan,

  Thank you for review. I fixed all the nits that you found.
I will address second round of feedback from Serguei, and then post an 
updated webrev.

Misha

On 9/14/16, 2:33 PM, Daniel D. Daugherty wrote:
> 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
>>>
>