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

[Ioi Lam]

Hi Calvin,

Looks good. Thanks for considering my suggestions.

- Ioi


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