RFR(S/M) : 8243565 : some gc tests use 'test.java.opts' and not 'test.vm.opts'

[Stefan Karlsson]

On 2020-04-27 18:07, Igor Ignatyev wrote:
>
>
>> On Apr 27, 2020, at 3:02 AM, Stefan Karlsson 
>> <stefan.karlsson at oracle.com <mailto:stefan.karlsson at oracle.com>> wrote:
>>
>> On 2020-04-25 01:06, Igor Ignatyev wrote:
>>> http://cr.openjdk.java.net/~iignatyev/8243565/webrev.00/
>>>> 335 lines changed: 12 ins; 191 del; 132 mod;
>>> Hi all,
>>> could you please review the patch which updates a dozen of gc tests 
>>> to pass both 'test.java.opts' and 'test.vm.opts' to JVM under test? 
>>> to achieve that, the patch removes code which was reading 
>>> test.java.opts and joing it w/ other flags, and instead uses 
>>> ProcessTools.createJavaProcessBuilder(boolean 
>>> addTestVmAndJavaOptions=true, String...), , which prepends values of 
>>> both properties. in some tests, it led to removal of diagnostic 
>>> output, this output is redundant as ProcessTools/OutputAnalyzer not 
>>> only prints out information about failed processes, but also dumps 
>>> it in working directory (see 8174768).
>>> the patch also removes redundant @modules, and replaces @run main w/ 
>>> @run driver in all these tests as there is no need of the "main" 
>>> test classes to be run w/ any of external flags. also in order to 
>>> reduce confusion $XOpts variables were renamed to $XFlags.
>>> from JBS:
>>>> a dozen of gc tests use 'test.java.opts' to get external vm flags, 
>>>> yet actually external flags are split b/w 'test.java.opts' and 
>>>> 'test.vm.opts' and in the majority of cases all external flags are 
>>>> set listed in 'test.vm.opts'. so the tests should be updated to 
>>>> read both.
>>> webrev: http://cr.openjdk.java.net/~iignatyev/8243565/webrev.00
>>
>> The code changes looks good. I don't understand the @module parts, so 
>> that needs to be reviewed by someone else.
>>
>> StefanK
>
> thanks for review Stefan!
>
> I can try to explain @modules part to you :
>  - in most cases I removed '@modules java.base/jdk.internal.misc' 
> which was adding --add-export flag so jdk.internal.misc package was 
> available for a test. if a test really depended on classes from 
> jdk.internal.misc, it would fail to compile
> - in TestHumongousCodeCacheRoots, I also removed 
> '@modules java.management' which simply was telling jtreg to run this 
> test only if java.management module exists and is available. the test 
> doesn't use any of API provided by  java.management (directly or 
> indirectly) so this tag can be safely removed.
>
> all these @modules were needed before, b/c some of generally used 
> jdk.test.lib. classes have dependencies on jdk.internal.misc package 
> and java.management module, but these dependencies got removed by 
> 8164737 and 8164944, and there are 8185163 and 8178416 to clean up 
> @modules in our tests. so if you prefer, I can exclude @modules 
> changes from 8243565 and send them out as a fix for another RFE.

If you move the modules cleanup to a separate RFE, you can consider the 
rest reviewed.

Thanks,
StefanK

>
> -- Igor
>
>>
>>> JBS:https://bugs.openjdk.java.net/browse/JDK-8243565
>>> Thanks,
>>> -- Igor
>