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

[Ioi Lam]

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