Hi Artem,
The test is updated again:
http://cr.openjdk.java.net/~jjiang/8186057/webrev.06/
The report table is decorated.
And please see more comments inline.
On 24/11/2017 01:06, Artem wrote:
Hi John,
A couple of more (probably minor) comments. Addressing them might
simplify the code and make it more readable. Although, one might argue
with it. So, feel free to ignore them. The only thing which I would
like to clarify is the questing about timeouts below.
I see you use multi-lined boolean expressions and ternary operators
(especially when they are long). It might make the code shorter, but I
believe in most cases it doesn't make it more readable.
If not using ternary operator (conditional operator), it has to use
if-else block. That would not be better.
Especially, I don't use complex expressions with conditional operator.
And the coding indent should comply with our convention.
If you really want to make the code shorter (I don't think it should
be the goal), you can also consider dropping "else" blocks if you just
return values.
String (String arg) {
if (args.equals("foo")) {
return 'F';
} else if (args.equals("bar")) {
return "B";
} else {
return "";
}
}
Do you mean change the above codes to the below style?
String (String arg) {
if (args.equals("foo")) {
return 'F';
} else if (args.equals("bar")) {
return "B";
}
return "";
}
Frankly, I'm not sure which one is better.
I would also avoid implicit converting an integer to a string by
concatenation of the integer with an empty string. You can use
String.valueOf() instead.
If that, do you suggest to change the following codes (in
Compatibility.java) to use String.valueOf() for boolean values?
101 props.put(Utils.PROP_SUPPORTS_SNI_ON_SERVER,
102 serverJdk.supportsSNI + "");
103 props.put(Utils.PROP_SUPPORTS_ALPN_ON_SERVER,
104 serverJdk.supportsALPN + "");
Here, serverJdk.supportsSNI and serverJdk.supportsALPN are boolean values.
128 props.put(Utils.PROP_PORT, port + "");
Here, port is an integer, but many existing JDK codes just use [port +
""] instead of [String.valueOf(port)].
Please also see inline.
On 11/23/2017 04:54 PM, sha.ji...@oracle.com wrote:
Hi John,
Could you please upload the report to
http://cr.openjdk.java.net/~jjiang/8186057 ?
http://cr.openjdk.java.net/~jjiang/8186057/webrev.05/report/report.html
Thank you. It might be better to add some space between columns.
I add a style for the table and make it more friendly.
http://cr.openjdk.java.net/~jjiang/8186057/webrev.06/report/report.html
One question about Compatibility.caseStatus(). What's the case when
you expect a timeout and no client output? Should it always result
to a test case failure?
I'm not sure understanding your question. Did you mean server or
client timeout?
I think client should output something.
My questing is - should timeout be always considered as a failure? Why
don't you just check if server or client result is FAIL?
if (clientStatus == FAIL || serverStatus == FAIL) {
return FAIL;
}
return PASS;
Some cases are negative cases, their status should be EXPECTED_FAIL.
A server or client timeout would lead to the case to fail, if the
failure is not expected.
If a server starts normally, but client doesn't start due to a negative
case, the server should get timeout. But this failure is expected.
If a server starts normally, and client get a unexpected timeout, this
failure would make the case to fail.
Otherwise, looks fine to me.
A couple of minor comments which you may want to address:
4. "sequence" field of JdkRelease looks redundant. A enum already
defines the order. The same with Protocol.
It looks the ordinal of enum constants is not recommended [1].
[1]
https://docs.oracle.com/javase/9/docs/api/java/lang/Enum.html#ordinal--
I don't see that it says that using "ordinal" is not recommended. I
think "sequence" just duplicates of "ordinal".
The following words are copied from the description of method
Enum.ordinal():
"Most programmers will have no use for this method. It is designed for
use by sophisticated enum-based data structures, such as EnumSet and
EnumMap."
In addition, Effective Java (2nd) Item #33 also advise us not using
ordinal index.
Best regards,
John Jiang
