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

[Mikhailo Seledtsov]

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 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 prefer the latter as well.
>
> 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
>
Thank you for this suggestion. I agree, we should try to factor boiler 
plate code into test utils where possible. I will add your 
recommendation to the list of CDSTestUtils improvements round 2.

Thank you,
Misha
> 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
>>>
>