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

Mikhailo Seledtsov mikhailo.seledtsov at oracle.com
Fri Aug 10 21:44:42 UTC 2018



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


More information about the hotspot-runtime-dev mailing list