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

[sangheon.kim at oracle.com]

Hi Igor,

On 1/28/19 6:32 PM, Igor Ignatyev wrote:
> Hi Sangheon,
>
> the code looks good to me.
Thank you for your review!
Forgot to mention, thank you for providing the idea of using @requires 
as well.

>
> joining bikeshedding around the property name, I'd recommend not to use 'vm.' at all, as it was intended to mean "VM under test can/have/...", so I'd go w/ something like 'test.vm.nvdimm' or 'test.vm.gc.nvdimm'. if you really want you can have ".enabled" prefix, but in this case, it seems to be redundant.
I changed to 'test.vm.gc.nvdimm'.

http://cr.openjdk.java.net/~sangheki/8217666/webrev.1/
(as I feel incremental webrev doesn't help, didn't generated)

Thanks,
Sangheon


>
> -- Igor
>
>> On Jan 28, 2019, at 11:31 AM, Leo Korinth <leo.korinth at oracle.com> wrote:
>>
>>
>>
>> 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