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

[Mikhailo Seledtsov]

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