RFR (smallish?) 8061611: Remove deprecated command line flags

[Bengt Rutisson]

Hi Derek,

On 2014-12-15 23:24, Derek White wrote:
> FYI -
>
> New webrev shows splitting regression test into simpler alias and 
> deprecation tests...
>
> http://cr.openjdk.java.net/~drwhite/8061611/webrev.03/

Thanks for fixing up the tests.

We don't use bug numbers in the test names. Please just name the tests 
GCDeprecatedOptions.java and GCAliasOptions.java.

GCDeprecatedOptions:

I'm not sure I think we need the test to verify that the test works.

   93     String[][] testTestDeprecated = {{"UseTLAB", "+"}};
   94     testDeprecated(testTestDeprecated, false); // Test the test. 
The output should NOT mention "UseTLAB" at all.

But if you feel better leaving it in, I guess I'm fine with it.

I don't understand this:

   81       String realOpt = (deprecated.length == 2) ? deprecated[0] : 
deprecated[1];
   82       String match = realOpt;

deprecatedc.length is always 2, right? And why do we need the 
intermediate variable realOpt?

I'm also a little concerned about this comment:

   78       // Searching precisely for deprecation warnings is too hard 
at the moment.
   79       // There is no standard format. For now, just search for the 
option name in the output,
   80       // which should only be printed if there was some 
deprecation warning.

There are only 5 deprecated options, so it seems like we should be able 
to do stricter checks.

What do you think about simply doing what we have done for other 
deprecated options? Just start a process and check the output:

http://hg.openjdk.java.net/jdk9/hs-gc/hotspot/file/bdf65c8bc1a9/test/gc/startup_warnings/TestDefaultMaxRAMFraction.java

I think it would be fine to add one test for each of the newly 
deprecated options, but I guess it is also possible to check all 5 
options in one test. Still the approach that 
TestDefaultMaxRAMFraction.java takes seems much simpler.



GCAliasOptions:

Similarlly to in GCDeprecatedOptions I am not convinced about the self test:

  105     // test the test:
  106     String[][] testTestAliases = {{"MarkStackSizeMax", 
"CMSMarkStackSizeMax", "1032"}};
  107     testAliases(testTestAliases, false); // MarkStackSizeMax is 
NOT an alias for CMSMarkStackSizeMax.

About DiagnosticMXBean. I realize that it will not be possible to verify 
the "=" vs. ":=" semantics if you use the MXBean instead of parsing the 
PrintFlagsFinal output. But on the other hand I am not so sure this is 
an important distinction to test as long at the values are correct. I'm 
fine with parsing the output if you think this is important. The MXBean 
approach would be fewer lines of code though.

If you go with the PrintFlagsFinal version I think it would be nice to 
make the ALIAS_OPTIONS contain small objects instead of String[]. Then 
you could have named getters instead of using alias[0], alias[1] and 
alias[2] in testAliases(). That would make it easier to understand.

Thanks,
Bengt

>
>  - Derek
>
> On 12/15/14 1:04 PM, Derek White wrote:
>> Thanks Bengt - comments below...
>>
>> On 12/15/14 7:51 AM, Bengt Rutisson wrote:
>>> Hi Derek,
>>>
>>> On 2014-12-12 15:35, Derek White wrote:
>>>> This is a request for final re-review.
>>>>
>>>> Updated for comments, and added regression test for arguments. This 
>>>> test is a bit overkill for this bug, but it's extensible for other 
>>>> deprecated and aliased options.
>>>>
>>>> Re-merged with tree.
>>>>
>>>> Webrev: http://cr.openjdk.java.net/~drwhite/8061611/webrev.01/
>>>
>>> The code changes look good to me.
>>>
>>> A few comments about the test.
>>>
>>> I don't think it is worth testing that the removed flags produce 
>>> error messages. Starting a VM takes some time and doing it 13 times 
>>> just to check that no one by mistake added the flags back seems like 
>>> a waste of resources to me. The risk that the flags are suddenly 
>>> added back is almost zero, right?
>> OK.
>>> I am also not convinced that a general and extensible test is the 
>>> way to go here. I think I would prefer a more specific test for the 
>>> flags that were deprecated now. For tests I think it is more 
>>> important that the tests are easily readable and understandable than 
>>> that code duplication is avoided. Thus, I strongly prefer small but 
>>> specific tests that clearly communicates what went wrong when they 
>>> fail and are easy to understand how they failed. So, in this case 
>>> maybe we should even have two tests? TestDeprecatedMarkStackFlags 
>>> and TestDeprecatedConcGCThreadsFlags.
>>
>> OK. I'm trying to calibrate how much testing is appropriate and how 
>> it should be organized. In fact, the other organizing principle was 
>> to be more "process oriented" - one (or more) regression tests per 
>> bug fixed. And regression tests would be essentially immutable - 
>> never expanded to test new cases. Is that about right?
>>
>> So to make the tests simpler, but bug specific, how about I break 
>> them up for one test to test aliases, and one test to test 
>> deprecation warnings?
>>> Instead of parsing the PrintFlagsFinal output you could use 
>>> ManagementFactoryHelper.getDiagnosticMXBean().getVMOption() to get 
>>> the option values and check if they are set correctly. I think that 
>>> will reduce the number of lines in the test further.
>>
>> PrintFlagsFinal has the added benefit of noting that a flag was 
>> explicitly set vs. has a default value. (":=" vs "="). I first used 
>> bizzare option values to try to test that, but ":=" is definitive.
>>
>> BTW, a motivation for the alias tests was thinking ahead to the redo 
>> of "alias" options that I'm planning. I'll want to test that the new 
>> alias handling code actually works for the remaining aliased options. 
>> But that can get it's own test file.
>>
>> Thanks again!
>>
>>  - Derek
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20141216/17541d5e/attachment.htm>