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

Mikhailo Seledtsov mikhailo.seledtsov at oracle.com
Tue Apr 25 15:41:11 UTC 2017


Looks good to me,

Thank you,
Misha

On 4/24/17, 11:28 PM, Ioi Lam wrote:
> 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
>>>>>
>>>
>


More information about the hotspot-runtime-dev mailing list