RFR[10] 8186057: TLS interoperability testing between different Java versions
sha.jiang at oracle.com
sha.jiang at oracle.com
Fri Nov 24 05:57:53 UTC 2017
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.jiang at 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
More information about the security-dev
mailing list