<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<br>
Hi Derek,<br>
<br>
<div class="moz-cite-prefix">On 2014-12-15 23:24, Derek White wrote:<br>
</div>
<blockquote cite="mid:548F5FA6.80300@oracle.com" type="cite">
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
<div class="moz-cite-prefix">FYI - <br>
<br>
New webrev shows splitting regression test into simpler alias
and deprecation tests...<br>
<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Edrwhite/8061611/webrev.03/">http://cr.openjdk.java.net/~drwhite/8061611/webrev.03/</a><br>
</div>
</blockquote>
<br>
Thanks for fixing up the tests.<br>
<br>
We don't use bug numbers in the test names. Please just name the
tests GCDeprecatedOptions.java and GCAliasOptions.java.<br>
<br>
GCDeprecatedOptions:<br>
<br>
I'm not sure I think we need the test to verify that the test works.<br>
<br>
93 String[][] testTestDeprecated = {{"UseTLAB", "+"}};<br>
94 testDeprecated(testTestDeprecated, false); // Test the
test. The output should NOT mention "UseTLAB" at all.<br>
<br>
But if you feel better leaving it in, I guess I'm fine with it.<br>
<br>
I don't understand this:<br>
<br>
81 String realOpt = (deprecated.length == 2) ? deprecated[0]
: deprecated[1];<br>
82 String match = realOpt;<br>
<br>
deprecatedc.length is always 2, right? And why do we need the
intermediate variable realOpt?<br>
<br>
I'm also a little concerned about this comment:<br>
<br>
78 // Searching precisely for deprecation warnings is too
hard at the moment.<br>
79 // There is no standard format. For now, just search for
the option name in the output,<br>
80 // which should only be printed if there was some
deprecation warning.<br>
<br>
There are only 5 deprecated options, so it seems like we should be
able to do stricter checks.<br>
<br>
What do you think about simply doing what we have done for other
deprecated options? Just start a process and check the output:<br>
<br>
<a class="moz-txt-link-freetext" href="http://hg.openjdk.java.net/jdk9/hs-gc/hotspot/file/bdf65c8bc1a9/test/gc/startup_warnings/TestDefaultMaxRAMFraction.java">http://hg.openjdk.java.net/jdk9/hs-gc/hotspot/file/bdf65c8bc1a9/test/gc/startup_warnings/TestDefaultMaxRAMFraction.java</a><br>
<br>
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.<br>
<br>
<br>
<br>
GCAliasOptions:<br>
<br>
Similarlly to in GCDeprecatedOptions I am not convinced about the
self test:<br>
<br>
105 // test the test:<br>
106 String[][] testTestAliases = {{"MarkStackSizeMax",
"CMSMarkStackSizeMax", "1032"}};<br>
107 testAliases(testTestAliases, false); // MarkStackSizeMax is
NOT an alias for CMSMarkStackSizeMax.<br>
<br>
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.<br>
<br>
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.<br>
<br>
Thanks,<br>
Bengt<br>
<br>
<blockquote cite="mid:548F5FA6.80300@oracle.com" type="cite">
<div class="moz-cite-prefix"> <br>
- Derek<br>
<br>
On 12/15/14 1:04 PM, Derek White wrote:<br>
</div>
<blockquote cite="mid:548F22A5.6020308@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<div class="moz-cite-prefix">Thanks Bengt - comments below...<br>
<br>
On 12/15/14 7:51 AM, Bengt Rutisson wrote:<br>
</div>
<blockquote cite="mid:548ED94B.6090506@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
Hi Derek,<br>
<br>
<div class="moz-cite-prefix">On 2014-12-12 15:35, Derek White
wrote:<br>
</div>
<blockquote cite="mid:548AFD2A.8060002@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<div class="moz-cite-prefix">This is a request for final
re-review.<br>
<br>
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.<br>
<br>
Re-merged with tree. <br>
<br>
Webrev: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Edrwhite/8061611/webrev.01/">http://cr.openjdk.java.net/~drwhite/8061611/webrev.01/</a><br>
</div>
</blockquote>
<br>
The code changes look good to me.<br>
<br>
A few comments about the test.<br>
<br>
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?<br>
</blockquote>
OK.<br>
<blockquote cite="mid:548ED94B.6090506@oracle.com" type="cite">
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.<br>
</blockquote>
<br>
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?<br>
<br>
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?<br>
<blockquote cite="mid:548ED94B.6090506@oracle.com" type="cite">
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.<br>
</blockquote>
<br>
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.<br>
<br>
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.<br>
<br>
Thanks again!<br>
<br>
- Derek<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>