[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR(xs): 8179103: [Testbug] re-enable the runtime/SharedArchiveFile/BootAppendTests.java test
From:       Calvin Cheung <calvin.cheung () oracle ! com>
Date:       2017-04-25 17:07:43
Message-ID: 58FF825F.2040809 () oracle ! com
[Download RAW message or body]

Thanks Ioi and Misha.
The test results look good and I'm going to push the change.

Calvin

On 4/25/17, 8:41 AM, Mikhailo Seledtsov wrote:
> 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@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
>>>>>>
>>>>
>>
[prev in list] [next in list] [prev in thread] [next in thread]