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

[Artem]

Hi John,

Please see inline.


On 11/28/2017 05:35 AM, sha.jiang at oracle.com wrote:
> Hi Artem,
> Please review the new webrev at: 
> http://cr.openjdk.java.net/~jjiang/8186057/webrev.07/
> Please see my comments below.
>
>>>>>>
>>>>>> 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.
> 1. This test designs 5 status:
> SUCCESS, UNEXPECTED_SUCCESS, FAIL, EXPECTED_FAIL and TIMEOUT
> The main purpose is making the test report more clear.
> A case fails that may be due to:
> Unexpected failure;
> No failure, but it is expected to get some failure;
> Timeout, although it is also not expected, this failure may have 
> nothing to do with SSL/TLS exceptions, like SSLHandshakeException.
> The test just wants to distinguish these different situations. I 
> assume users generally just look through the report table, but not the 
> detail logs.
>
I am not sure it's useful to distinguish this situations, but it makes 
the logic more complex.
> 2. This test really treats TIMEOUT as failure. Server and/or client 
> timout, it should result in the case gets FAIL or EXPECTED_FAIL.
>
> 3. Your above code snippet doesn't cover all possible result status.
> For example, client == PASS && server == PASS, but this case may be 
> expected to get some failure. The status should be UNEXPECTED_SUCCESS 
> in my design.
My code might not cover all cases because it's just an example. If there 
are more statuses, then it's going to be more complex. I am suggesting 
to get rid of unnecessary statuses to make it simpler.
>
> Again, some more status are used to make the report table could give 
> more information before investigating details.
I don't think it's going to be useful at least to me. Instead, it might 
be better to print an exception message in a separate columns for both 
client and server.
>
> 4. The updated webrev havs merged two if-clauses to simplify the 
> method Compatibility.caseStatus() a bit.
That's fine. I'll let you decide if you want to simplify it or not.

By the way, I just noticed that JdkUtils.supportECKey() method and other 
return strings "true" and "false" instead of boolean values. This looks 
a bit unusual and unnecessary.

Utils.java, line 146: it would be better to close the stream.

Artem
>
> Best regards,
> John Jiang