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

[Leo Korinth]

Still looks good.

Thanks,
Leo

On 29/01/2019 06:08, sangheon.kim at oracle.com wrote:
> 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
>