RFR(M): 8197959: [TESTBUG] Some (App)CDS tests requires modification due to there removal of the Java EE and CORBA modules

[Calvin Cheung]

Thanks, Misha.

Calvin

On 4/24/18, 8:12 PM, Mikhailo Seledtsov wrote:
> Looks good to me,
>
> Misha
>
> On 4/24/18, 4:44 PM, Calvin Cheung wrote:
>> Hi Ioi,
>>
>> Thanks for your review.
>>
>> On 4/19/18, 10:29 PM, Ioi Lam wrote:
>>> Hi Calvin,
>>>
>>> the changes look good. I have a few comments:
>>>
>>> [1] I think "@modules java.compiler" be added to all tests that use 
>>> javax/annotation, which is provided by the java.compiler module.
>> I've added the missing "@modules java.compiler".
>>>
>>>
>>> [2] CheckAnonymousClass.java:
>>>
>>> I believe the old behavior was that when "--add-modules java.corba" 
>>> was specified during dump time, some Lambda expressions were 
>>> executed and thus some VM anonymous classes were loaded as a side 
>>> effect.
>>>
>>> With the new version, I don't think any Lambda expressions are 
>>> executed during dump time (because the normal module bootstrap code 
>>> has been very careful in avoiding Lambda). Therefore, the new 
>>> version doesn't test the intended behavior.
>>>
>>> The AnonVmClassesDuringDump.java test forces Lambda expressions to 
>>> be executed during dump time (by using Java agents). Maybe the 
>>> pattern matching in  CheckAnonymousClass.java can be moved inside 
>>> AnonVmClassesDuringDump.java? Then, we can delete 
>>> CheckAnonymousClass.java
>> I've moved the pattern matching logic from CheckAnonymousClass.java 
>> to AnonVmClassesDuringDump.java and delete the CheckAnonymousClass.java.
>>>
>>> [3] For this code:
>>>
>>> +        CDSTestUtils.Result res = TestCommon.runWithModules(
>>> +                                      prefix,
>>> + UPGRADEDMODS_DIR[upgradeModIdx].toString(),
>>> +                                      MODS_DIR.toString(),
>>> +                                      mid,
>>> +                                      archiveClass, loaderName, 
>>> "true"); // last 3 args passed to test
>>> +
>>> +        output = res.getOutput();
>>> +        try {
>>> +            output.shouldContain(expectedException);
>>> +        } catch (Exception e) {
>>> +            TestCommon.checkCommonExecExceptions(output, e);
>>>          }
>>>
>>>    Is it because you don't care about the exit code? If so, you can 
>>> do this:
>>>
>>>
>>>         TestCommon.runWithModules(...)
>>>           .ifNoMappingFailure(out -> 
>>> out.shouldContain(expectedException));
>>>
>>>
>>>     Then you don't need to directly access the output.
>> I've also made similar changes to another usage of 
>> TestCommon.runWithModules() at lines 203-209 so that the 
>> CDSTestUtils.getOuput method is no longer needed.
>>
>> Updated webrevs:
>>
>>     incremental: 
>> http://cr.openjdk.java.net/~ccheung/8197959/webrev.00_01/
>>
>>     full: http://cr.openjdk.java.net/~ccheung/8197959/webrev.01/
>>
>> thanks,
>> Calvin
>>>
>>> Thanks
>>> - Ioi
>>>
>>>
>>> On 4/18/18 3:32 PM, Calvin Cheung wrote:
>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8197959
>>>>
>>>> webrev: http://cr.openjdk.java.net/~ccheung/8197959/webrev.00/
>>>>
>>>> Fixing the affected CDS and AppCDS tests due to the removal of the 
>>>> Java EE and CORBA modules.
>>>> Also removing the unneeded helper classes.
>>>>
>>>> It passed hs-tier1 through tier3 tests.
>>>>
>>>> thanks,
>>>> Calvin
>>>