RFR(xs): 8179103: [Testbug] re-enable the runtime/SharedArchiveFile/BootAppendTests.java test

[Ioi Lam]

Looks good. Thanks Calvin!

- Ioi

On 4/25/17 12:15 PM, Calvin Cheung wrote:
>
>
> On 4/24/17, 8:07 PM, Ioi Lam wrote:
>>
>>
>> On 4/25/17 8:32 AM, Calvin Cheung wrote:
>>> Hi Ioi,
>>>
>>> Thanks for taking a look.
>>>
>>> On 4/24/17, 4:40 PM, Ioi Lam wrote:
>>>> I think it's better to use CDSTestUtils.checkExec which will check 
>>>> a bunch of other conditions:
>>>>
>>>> OutputAnalyzer out = CDSTestUtils.runWithArchive(opts);
>>>> CDSTestUtils.checkExec(out, "java.lang.ClassNotFoundException: 
>>>> javax.sound.sampled.MyClass");
>>>
>>> CDSTestUtils.checkExec() has the following check:
>>>             if ("on".equals(opts.xShareMode)) {
>>>                 output.shouldContain("sharing");
>>>             }
>>>
>>> It works with tests running with the "-version" because the 
>>> "sharing" is from the version string.
>>>
>>> In the BootAppendTest.java, the tests don't use the "-version" 
>>> option so the checkExec() doesn't work well here.
>>>
>> The -showversion option will print the version string during VM start 
>> up, before running the specified class. If you look at the closed CDS 
>> tests, you can see all of them are run with -showversion so we can 
>> check for the "sharing" string.
> I forgot about -showversion. Thanks for pointing that out.
>>
>>> I could use the following:
>>> CDSTestUtils.checkExecExpectError(out, 0, 
>>> "java.lang.ClassNotFoundException: javax.sound.sampled.MyClass");
>>> but the name is a bit confusing if there's no error expected.
>>>
>>> Also, for Test #4, it requires the "out.shouldMatch":
>>>             if (!CDSTestUtils.isUnableToMap(out)) {
>>>                 out.shouldContain("[class,load] 
>>> org.omg.CORBA.Context");
>>>                 out.shouldMatch(".*\\[class,load\\] 
>>> org.omg.CORBA.Context source:.*bootAppend.jar");
>>>             }
>>>
>>> The checkExecExpectError() calls checkExtraMatches() which only 
>>> calls output.shouldContain. Therefore, the checkExecExpectError() 
>>> can't be used for the "out.shouldMatch" case.
>>>
>>> I'd prefer to leave the change as is (webrev.01).
>>>
>>
>> There are a few problems with this:
>>
>> 1. isUnableToMap doesn't validate that the mapping actually 
>> succeeded. It just check that mapping did not fail, which doesn't 
>> mean the same thing -- For example, your test might have forgotten to 
>> specify -Xshare:on, in which case mapping never happened, so it never 
>> failed, but you didn't test what you wanted to test.
>>
>> 2. The test could have crashed or thrown an exception, after print 
>> the "shouldContain" string. So you should at least do this:
>>
>>            if (!CDSTestUtils.isUnableToMap(out)) {
>>                 out.shouldContain("[class,load] org.omg.CORBA.Context");
>>                 out.shouldMatch(".*\\[class,load\\] 
>> org.omg.CORBA.Context source:.*bootAppend.jar");
>>                 out.shouldHaveExitValue(0)
>>             }
>>
>> I think it's better to add -showversion to the parameters, and then
>>
>>             CDSTestUtils.checkExec(out, "[class,load] 
>> org.omg.CORBA.Context");
>>             if (!CDSTestUtils.isUnableToMap(out)) {
>>                 out.shouldMatch(".*\\[class,load\\] 
>> org.omg.CORBA.Context source:.*bootAppend.jar");
>>             }
>
> I implemented the latter.
>
> I needed to use the checkExec() with 3 args:
> public static OutputAnalyzer checkExec(OutputAnalyzer output, 
> CDSOptions opts,
>                                      String... extraMatches)
>
> Otherwise, a CDSOptions object will be created with the default 
> xShareMode set to "on".
>
> Here's an updated webrev:
> http://cr.openjdk.java.net/~ccheung/8179103/webrev.02/
>
> Testing on other platforms is underway.
>>
>> I think in general it's better to do the test result validation in a 
>> more controlled way, and avoid all the boiler-plate code such as 
>> isUnableToMap(). We should improve CDSTestUtils so that we can write 
>> tests like this:
>>
>>    CDSTestUtils.checkExec(out, (out) -> {
>>         out.shouldContain("xxxxx")
>>    .shouldNotContain("yyyy");
>>              .shouldMatch(".*zzzzz");
>>         });
>>
>> This way the boiler-plate checks are done first, and then the 
>> test-specific validation code can be executed. I've filed 
>> https://bugs.openjdk.java.net/browse/JDK-8179249
> I think the CDSOptions can be improved as well. I'll add to the bug 
> report.
>
> thanks,
> Calvin
>>
>> Thanks
>> - Ioi
>>
>>
>>> Let me know if it's fine with you.
>>>
>>> thanks,
>>> Calvin
>>>
>>>>
>>>> Thanks
>>>> - Ioi
>>>>
>>>>
>>>> On 4/25/17 12:18 AM, Calvin Cheung wrote:
>>>>> I turns out that the fix needs to be updated to account for the 
>>>>> "unable to map archive" error on platfoms such as windows 32-bit. 
>>>>> The following updated webrev passed all hotspot_runtime tests on 
>>>>> all applicable platforms.
>>>>>
>>>>> http://cr.openjdk.java.net/~ccheung/8179103/webrev.01/
>>>>>
>>>>> Change pattern is as follows:
>>>>>
>>>>> before:
>>>>>              CDSTestUtils.runWithArchive(opts)
>>>>> .shouldContain("java.lang.ClassNotFoundException: 
>>>>> javax.sound.sampled.MyClass");
>>>>>
>>>>> after:
>>>>>              OutputAnalyzer out = CDSTestUtils.runWithArchive(opts);
>>>>>              if (!CDSTestUtils.isUnableToMap(out)) {
>>>>> out.shouldContain("java.lang.ClassNotFoundException: 
>>>>> javax.sound.sampled.MyClass");
>>>>>              }
>>>>>
>>>>> thanks,
>>>>> Calvin
>>>>>
>>>>> On 4/21/17, 6:25 PM, Calvin Cheung wrote:
>>>>>> Thanks Jiangli!
>>>>>> I'll wait until the testing is done before pushing the fix.
>>>>>>
>>>>>> thanks,
>>>>>> Calvin
>>>>>>
>>>>>> On 4/21/17, 6:06 PM, Jiangli Zhou wrote:
>>>>>>> Looks good.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Jiangli
>>>>>>>
>>>>>>>> On Apr 21, 2017, at 3:18 PM, Calvin 
>>>>>>>> Cheung<calvin.cheung at oracle.com> wrote:
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Please review this simple fix for re-enabling a test case.
>>>>>>>>
>>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8179103
>>>>>>>>
>>>>>>>> webrev: http://cr.openjdk.java.net/~ccheung/8179103/webrev.00/
>>>>>>>>
>>>>>>>> Tested locally on linux-x64.
>>>>>>>> Running tests on other platforms.
>>>>>>>>
>>>>>>>> thanks,
>>>>>>>> Calvin
>>>>
>>