<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">Hi Jesper,<br>
<br>
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.<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~drwhite/8061611/webrev.04">http://cr.openjdk.java.net/~drwhite/8061611/webrev.04</a><br>
<br>
<meta http-equiv="content-type" content="text/html; charset=utf-8">
GCAliasOptions8061611.java vs. VMAliasOptions.java<br>
GCDeprecatedOptions8061611.java vs. VMDeprecatedOptions.java<br>
<br>
Please let me know if you have any thoughts on this.<br>
<br>
- Derek<br>
<br>
On 12/16/14 6:04 AM, Jesper Wilhelmsson wrote:<br>
</div>
<blockquote cite="mid:549011AB.90406@oracle.com" type="cite">Looks
good!
<br>
<br>
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?
<br>
/Jesper
<br>
<br>
<br>
Derek White skrev 15/12/14 23:24:
<br>
<blockquote type="cite">FYI -
<br>
<br>
New webrev shows splitting regression test into simpler alias
and deprecation
<br>
tests...
<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~drwhite/8061611/webrev.03/">http://cr.openjdk.java.net/~drwhite/8061611/webrev.03/</a>
<br>
<br>
- Derek
<br>
<br>
On 12/15/14 1:04 PM, Derek White wrote:
<br>
<blockquote type="cite">Thanks Bengt - comments below...
<br>
<br>
On 12/15/14 7:51 AM, Bengt Rutisson wrote:
<br>
<blockquote type="cite">Hi Derek,
<br>
<br>
On 2014-12-12 15:35, Derek White wrote:
<br>
<blockquote type="cite">This is a request for final
re-review.
<br>
<br>
Updated for comments, and added regression test for
arguments. This test is
<br>
a bit overkill for this bug, but it's extensible for other
deprecated and
<br>
aliased options.
<br>
<br>
Re-merged with tree.
<br>
<br>
Webrev:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~drwhite/8061611/webrev.01/">http://cr.openjdk.java.net/~drwhite/8061611/webrev.01/</a>
<br>
</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
<br>
messages. Starting a VM takes some time and doing it 13
times just to check
<br>
that no one by mistake added the flags back seems like a
waste of resources
<br>
to me. The risk that the flags are suddenly added back is
almost zero, right?
<br>
</blockquote>
OK.
<br>
<blockquote type="cite">I am also not convinced that a general
and extensible test is the way to go
<br>
here. I think I would prefer a more specific test for the
flags that were
<br>
deprecated now. For tests I think it is more important that
the tests are
<br>
easily readable and understandable than that code
duplication is avoided.
<br>
Thus, I strongly prefer small but specific tests that
clearly communicates
<br>
what went wrong when they fail and are easy to understand
how they failed.
<br>
So, in this case maybe we should even have two tests?
<br>
TestDeprecatedMarkStackFlags and
TestDeprecatedConcGCThreadsFlags.
<br>
</blockquote>
<br>
OK. I'm trying to calibrate how much testing is appropriate
and how it should
<br>
be organized. In fact, the other organizing principle was to
be more "process
<br>
oriented" - one (or more) regression tests per bug fixed. And
regression tests
<br>
would be essentially immutable - never expanded to test new
cases. Is that
<br>
about right?
<br>
<br>
So to make the tests simpler, but bug specific, how about I
break them up for
<br>
one test to test aliases, and one test to test deprecation
warnings?
<br>
<blockquote type="cite">Instead of parsing the PrintFlagsFinal
output you could use
<br>
ManagementFactoryHelper.getDiagnosticMXBean().getVMOption()
to get the option
<br>
values and check if they are set correctly. I think that
will reduce the
<br>
number of lines in the test further.
<br>
</blockquote>
<br>
PrintFlagsFinal has the added benefit of noting that a flag
was explicitly set
<br>
vs. has a default value. (":=" vs "="). I first used bizzare
option values to
<br>
try to test that, but ":=" is definitive.
<br>
<br>
BTW, a motivation for the alias tests was thinking ahead to
the redo of
<br>
"alias" options that I'm planning. I'll want to test that the
new alias
<br>
handling code actually works for the remaining aliased
options. But that can
<br>
get it's own test file.
<br>
<br>
Thanks again!
<br>
<br>
- Derek
<br>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</body>
</html>