RFR(S): 8209164: [TESTBUG] Apply jtreg skipped status to cds tests
mikhailo
mikhailo.seledtsov at oracle.com
Tue Aug 14 23:31:22 UTC 2018
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
>>>>>>>
>>>>
>
More information about the hotspot-runtime-dev
mailing list