RFR (smallish?) 8061611: Remove deprecated command line flags
Derek White
derek.white at oracle.com
Tue Dec 16 20:12:36 UTC 2014
Hi Bengt,
On 12/16/14 6:50 AM, Bengt Rutisson wrote:
> 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.
This gets back to whether these tests should be immutable and test
against a specific bug, or more like unit tests that are modified as
functionality changes. If the former then there would be a growing
number of test files that would be 99% identical, and they would need
unique names. But now I think this is a bad idea for these specific
tests - they should be modified as the implementation is modified.
I think the misunderstanding here is that the test cases were designed
to cover both the changes for 8061611 as well as testing the upcoming
re-implementation of alias and deprecation handling. But people can only
review the code in front of them, not some code from the future :-)
The goal of the re-implementation is to allow a developer to implement
an alias option or add a deprecation warning by adding one line of code
to a table (as opposed to ad-hoc code). The goal of these new tests was
to similarly allow a developer to add a test for an aliased or
deprecated option by adding one line of code to a table.
i.e If the implementation is driven by a global table, then the tests
should be as well. Let me know if you think this is a horrible idea.
So that's the end of explaining code */not/* under review :-)
For the code under review:
> 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.
OK. I'm a bit new to writing regular regression tests, so maybe I'm
getting carried away with the concept :-)
> 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?
OK. That was for future expansion.
> 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.
When I re-implement deprecation messages there will be a standard format
for these options which will be easier to test. So this code isn't
precise, but it's sufficient, and will be precise in the next version.
If that's OK.
> 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.
O, I'll try MXBeans.
>
> 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.
The reason for the String arrays was to better match the upcoming C
implementation and the Java test code:
/**
* Some flags are actually aliases for other flags. This is often
part of the process of
* deprecating a flag, but not all aliases need to be deprecated.
*/
typedef struct {
const char* alias_name;
const char* real_name;
} AliasedFlag;
static AliasedFlag aliased_jvm_flags[] = {
{ "DefaultMaxRAMFraction", "MaxRAMFraction"} //<- this line
can be pasted into VMAliasOptions.java & VMDeprecatedoptions.java
and updated.
}
So the goal was to make it trivial to add to the test case.
I'll get a new webrev out soon...
- 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/4dc04d11/attachment.htm>
More information about the hotspot-gc-dev
mailing list