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

[Derek White]

Hi Jesper,

After reading your comments and Bengt's, I tried two version of the 
tests - one with makeOptionArg() pushed up to CommandLineOptionTest in 
testLibrary, and another where almost all logic (creating command line 
flags, starting vm, and verifying flag values) is pushed up to 
CommandLineOptionTest.

http://cr.openjdk.java.net/~drwhite/8061611/webrev.04

GCAliasOptions8061611.java           vs. VMAliasOptions.java
GCDeprecatedOptions8061611.java vs. VMDeprecatedOptions.java

Please let me know if you have any thoughts on this.

  - Derek

On 12/16/14 6:04 AM, Jesper Wilhelmsson wrote:
> Looks good!
>
> One question, I'm fine with the change regardless of the answer; Both 
> tests uses makeOptionArg() and it is a generic method that could be 
> useful in other tests as well. Would it make sense to move it into the 
> testlibrary?
> /Jesper
>
>
> Derek White skrev 15/12/14 23:24:
>> FYI -
>>
>> New webrev shows splitting regression test into simpler alias and 
>> deprecation
>> tests...
>>
>> http://cr.openjdk.java.net/~drwhite/8061611/webrev.03/
>>
>>   - 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/cc721f4b/attachment.htm>