RFR: 8260349: Cannot programmatically retrieve Metaspace max set via JAVA_TOOL_OPTIONS [v2]

[David Holmes]

On Thu, 28 Jan 2021 09:57:59 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> David Holmes has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Boost max from 1MB to 9MB as 1MB was too low for Aarch64 to startup
>
> test/hotspot/jtreg/runtime/Metaspace/MaxMetaspaceSizeEnvVarTest.java line 36:
> 
>> 34:  * @library /test/lib
>> 35:  * @run driver MaxMetaspaceSizeEnvVarTest
>> 36:  */
> 
> I believe the conventional style is to do jtreg test definition the first thing, and then do imports.

Thanks for the Review Aleksey - note I just updated the test with a missing test case.

I'm not aware of a specific convention here. :) I used the existing MaxMetaspaceSizeTest.java in the same directory as the template for this one. But I can change it if you insist.

Thanks,
David

> test/hotspot/jtreg/runtime/Metaspace/MaxMetaspaceSizeEnvVarTest.java line 88:
> 
>> 86:                              "_JAVA_OPTIONS",
>> 87:                              "JAVA_TOOL_OPTIONS"
>> 88:         };
> 
> This is an odd style. Suggestion:
> 
> String[] envVars = { 
>     "JDK_JAVA_OPTIONS",
>     "_JAVA_OPTIONS",
>     "JAVA_TOOL_OPTIONS"
> };

Sure.

> test/hotspot/jtreg/runtime/Metaspace/MaxMetaspaceSizeEnvVarTest.java line 76:
> 
>> 74:         int test = 1;
>> 75:         String msg = "Test " + test +": normal command-line flag";
>> 76:         report(msg);
> 
> What is the point of declaring local variables `msg` here? AFAICS, it might as well be:
> 
> report("Test " + test +": normal command-line flag");

A leftover - I originally just printed to System.out but that doesn't capture the OutputAnalyzer text so I also wrote to System.err. Then decided to factor that out into report().

-------------

PR: https://git.openjdk.java.net/jdk/pull/2275