RFR(S): 8209164: [TESTBUG] Apply jtreg skipped status to cds tests

[Jiangli Zhou]

+1

The test changes look good. Thanks for collecting the statistics.

Thanks,

Jiangli


On 8/14/18 4:42 PM, Ioi Lam wrote:
> Sound good. Ship it!
>
> Thanks
>
> - Ioi
>
>
> On 8/14/18 4:31 PM, mikhailo wrote:
>> Hi Ioi,
>>
>>   I ran the tests (Win-x64, 10 times both SharedArchiveFile and 
>> appcds), only one test with "SkippedStatus". See the bug report 
>> comments for details.
>>
>> Also, I run a Jenkins job on a daily basis with several vmTestbase 
>> test sets with -Xshare:on, probably closed to 1000 tests, and only 1 
>> or 2 fail due to mapping failure, and not every day.
>>
>> Based on these stats, I believe this change should not negatively 
>> affect test coverage in any measurable way.
>>
>>
>> Misha
>>
>>
>>
>> On 08/10/2018 05:06 PM, Ioi Lam wrote:
>>> Hi Misha,
>>>
>>> The changes in CDSTestUtils look good to me.
>>>
>>> Thanks
>>>
>>> - Ioi
>>>
>>>
>>> On 8/10/18 3:22 PM, Mikhailo Seledtsov wrote:
>>>> Hi Ioi,
>>>>
>>>>  Here is the updated webrev that fixes the CDSTestUtils.Result to 
>>>> checkMappingFailure() first thing in the constructor, and updates 
>>>> the rest of the class accordingly.
>>>> http://cr.openjdk.java.net/~mseledtsov/8209164.03/index.html
>>>>
>>>> Please review to make sure we are on the same page with this 
>>>> change, specifically the CDSTestUtils.Result, before I begin 
>>>> multiple test runs on Windows.
>>>>
>>>>
>>>> Thank you,
>>>> Misha
>>>>
>>>> On 8/10/18, 2:44 PM, Mikhailo Seledtsov wrote:
>>>>>
>>>>>
>>>>> On 8/10/18, 2:13 PM, Ioi Lam wrote:
>>>>>> Hi Misha,
>>>>>>
>>>>>> Have you done a few Windows runs and see how many tests report 
>>>>>> being skipped?
>>>>> Will do.
>>>>>
>>>>> I understand your concern. A number of CDS/AppCDS tests are 
>>>>> structured around multiple test cases hosted in the same file, for 
>>>>> good reasons (such as combinatorial testing, logical grouping of 
>>>>> test cases, avoiding excessive code duplication). However, jtreg 
>>>>> only supports one result per test file (AFAIK). Hence, when 
>>>>> multiple test cases are hosted in a single file, and one test case 
>>>>> throws SkippedException because of mapping failure, the rest of 
>>>>> the test cases in that file do not get to execute.
>>>>>
>>>>> If it only happens once in a while, then it is OK to skip a few 
>>>>> test cases once in a while. If it happens too often, this will 
>>>>> reduce overall cds test coverage.
>>>>> Let me measure and see. I will do 10 runs on Windows, and check 
>>>>> for "skipped" status. Then we can decide what to do.
>>>>>
>>>>>
>>>>>> Also, unlike your previous version (which called 
>>>>>> checkCommonExecExceptions inside the Result constructor) now we 
>>>>>> have a divergence of behavior:
>>>>>>
>>>>>> + tests that use the CCDSTestUtils.run() APIs will silently 
>>>>>> ignore the mapping failures.
>>>>>>
>>>>>> + tests that use the old TestCommon.checkExec APIs will throw 
>>>>>> SkippedException.
>>>>>>
>>>>>> I think it's better to unify the behavior.
>>>>> I agree, we should use one pattern of behavior, and my preference 
>>>>> is on SkippedException.
>>>>> I will update the code, and post new webrev shortly.
>>>>>
>>>>>
>>>>> Thank you,
>>>>> Misha
>>>>>
>>>>>> Thanks
>>>>>>
>>>>>> - Ioi
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 8/10/18 1:40 PM, Mikhailo Seledtsov wrote:
>>>>>>> Here is the updated webrev: 
>>>>>>> http://cr.openjdk.java.net/~mseledtsov/8209164.02/
>>>>>>> Hopefully addressing all the feedback.
>>>>>>>
>>>>>>> Thank you,
>>>>>>> Misha
>>>>>>>
>>>>>>> On 8/9/18, 6:44 PM, Mikhailo Seledtsov wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 8/9/18, 4:22 PM, Ioi Lam wrote:
>>>>>>>>> Hi Misha,
>>>>>>>>>
>>>>>>>>> I think you should change checkCommonExecExceptions to a void 
>>>>>>>>> function, since it now always throws or returns false.
>>>>>>>>>
>>>>>>>>> That way, you can catch places that try to read the return 
>>>>>>>>> value, and fix these as appropriate. E.g., this in the Result 
>>>>>>>>> class
>>>>>>>>>
>>>>>>>>>             hasMappingFailure = 
>>>>>>>>> CDSTestUtils.checkCommonExecExceptions(output);
>>>>>>>>>             hasAbnormalExit   = (!hasMappingFailure) && 
>>>>>>>>> (output.getExitValue() != 0);
>>>>>>>>>             hasNormalExit     = (!hasMappingFailure) && 
>>>>>>>>> (output.getExitValue() == 0);
>>>>>>>>>
>>>>>>>>> I think the Result.hasMappingFailure field can now be removed.
>>>>>>>> Sounds good; I think I can fix that as well with this change.
>>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>>
>>>>>>>>> - Ioi
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 8/9/18 1:50 PM, Mikhailo Seledtsov wrote:
>>>>>>>>>> Please review this straight forward change. Now that JTReg 
>>>>>>>>>> harness supports the "skipped status",
>>>>>>>>>> this change updates CDS and AppCDS tests to use this 
>>>>>>>>>> mechanism where applicable.
>>>>>>>>>>
>>>>>>>>>>     JBS: https://bugs.openjdk.java.net/browse/JDK-8209164
>>>>>>>>>>     Webrev: 
>>>>>>>>>> http://cr.openjdk.java.net/~mseledtsov/8209164.01/index.html
>>>>>>>>>>     Testing:
>>>>>>>>>>         1. Locally: exercised tests under 
>>>>>>>>>> runtime/SharedArchiveFile and runtime/appcds on Linux-x64
>>>>>>>>>>            No new failures
>>>>>>>>>>     2. Run runtime/SharedArchiveFile and runtime/appcds via 
>>>>>>>>>> distributed test system
>>>>>>>>>>            Run tier1,tier2
>>>>>>>>>>            In progress
>>>>>>>>>>
>>>>>>>>>> Thank you,
>>>>>>>>>> Misha
>>>>>>>>>
>>>>>>
>>>
>>
>