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

[Ioi Lam]

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