RFR[10] 8186057: TLS interoperability testing between different Java versions

[Artem]

Hi John,

Let me skip most of cosmetic/style comments. I think you got my points, 
I'll let you decide if you want to address them or not. Or, you may want 
to ask for other's opinions.

Let's focus on the question about timeout below.

On 11/24/2017 08:57 AM, sha.jiang at oracle.com wrote:
>>>> 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
The report looks much better now, thank you.
>
>>>>
>>>> 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.
That's fine, I got that.
> 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.
This logic looks unnecessary complex to me. The more complex it is, the 
more likely we get a bug there. I think TIMEOUT status is not necessary 
here. If a failure is expected on one side, the other side should fail 
as well (no matter if it's a timeout or an exception).

If I were you, I would simplify this method like this

if (client == EXPECTED_FAIL && server == EXPECTED_FAIL) {
     return EXPECTED_FAIL;
}

if (client == PASS && server == PASS) {
     return PASS;
}

return FAIL;

If I am not missing something, the pseudo-code above looks better and 
simpler.
>
>>>
>>>>
>>>> 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."
I don't think it says that "ordinal" is not recommended to use :)
>
> In addition, Effective Java (2nd) Item #33 also advise us not using 
> ordinal index.
I respect and appreciate Mr. Bloch's book, but my point is that 
"ordinal" and "sequence" have exact the same meaning, so "sequence" just 
duplicates it, and as a result, it looks redundant :)

http://hg.openjdk.java.net/jdk10/master/file/be620a591379/src/java.base/share/classes/java/lang/Enum.java#l91

...
     private final int ordinal;
...
     public final int ordinal() {
         return ordinal;
     }
...

Artem
>
> Best regards,
> John Jiang