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

[Ioi Lam]

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

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