RFR (S)[12]: 8217666: gc/nvdimm/* should not be included any tiers

[Leo Korinth]

On 28/01/2019 20:09, sangheon.kim at oracle.com wrote:
> Hi Leo,
> 
> Thank you for reviewing this.
> 
> On 1/28/19 2:25 AM, Leo Korinth wrote:
>> Hi Sangheon,
>>
>> a few questions on the webrev:
>> 1) what does the added vm.nvdimm.test.enabled line in TEST.ROOT do?
> vm.nvdimm.test.enabled is added at the requires.properties list at 
> TEST.ROOT and this is necessary for its work flow.
> FYI, without that change, we will see 'Syntax error in @requires 
> expression: invalid name: vm.nvdimm.test.enabled'.
> 

Thank you for the explanation.

>> 2) most of the *Enabled() functions in VMProps.java read properties, 
>> but here we read the environment, why is that?
> I think the env. variable approach seems clearer to use considering 
> those tests will be tested on limited situations.
> JTREG and VM which runs requires.VMProps will be affected.
> JTREG doesn't propagate env variables to JDK under test unless they are 
> specified via -e flag.
> I considered adding a property as well, but I ended up with the 
> environment variable.
Okay, I just wanted to know that it was not by accident.

> 
>> 3) maybe the property should be "vm.gc" prefixed instead of "vm" 
>> prefixed, maybe not. What do you think?
> I'm okay with 'vm.gc' prefix.
> 'vm.gc.nvdimm.test.enabled' unless others dislike it. :) > Let me post the updated webrev after getting more comments.
I am fine with either. Your change looks good to me, and thanks a lot 
for fixing this problem. Keep in mind that I am just a commiter.

Thanks,
Leo
> 
> Thanks,
> Sangheon
> 
>>
>> Thanks for disabling these test cases!
>> /Leo
>>
>> On 26/01/2019 16:35, sangheon.kim at oracle.com wrote:
>>> Hi all,
>>>
>>> Can I have reviews that excludes gc/nvdimm jtreg tests?
>>>
>>> Those tests were introduced by  JDK-8202286 (Allocation of old 
>>> generation of Java heap on alternate memory devices) and tried to 
>>> exclude all tests from all tiers. But it was incomplete so one of the 
>>> tests failed and JDK-8217406 (gc/nvdimm/TestOldObjectsOnNvdimm.java 
>>> failure) was filed recently.
>>>
>>> The patch includes to exclude gc/nvdimm from TEST.groups, 
>>> hotspot_misc group(which is the reason why JDK-8217406 occurred). In 
>>> addition, added @requires to gc/nvdimm tests to avoid running the tests.
>>>
>>> CR: https://bugs.openjdk.java.net/browse/JDK-8217666
>>> webrev: http://cr.openjdk.java.net/~sangheki/8217666/webrev.0/
>>> Testing: manual tests w/, w/o VM_NVDIMM_TEST environmental variable.
>>>
>>> Thanks,
>>> Sangheon
>