Hi John,

Could you please upload the report to http://cr.openjdk.java.net/~jjiang/8186057 ?

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?

Otherwise, looks fine to me.

A couple of minor comments which you may want to address:

1. Cert.getCert(): it would be nice to add a comment which explains what's returned in the array. It might be good to add an "else" block and throw an exception there.

2. Client.java, lines 83-84: it's not necessary to print a stack trace if you put the original exception to runtime exception. The same with ProcessUtils.

3. JdkInfo: the fields of this class should be final since you don't modify them then. If compilation fails because of complex logic in constructor, then it may be a sign that the logic should be simplified. Or, you can put the logic to a static factory method.

4. "sequence" field of JdkRelease looks redundant. A enum already defines the order. The same with Protocol.

5.

On 11/14/2017 09:16 AM, sha.ji...@oracle.com wrote:
Hi Artem,
Please find the new webrev [1].
This test has been refactored significantly. It supports more cipher suites, and case combinations among the TLS communication parameters.
For more details, please look through README [2].

[1] http://cr.openjdk.java.net/~jjiang/8186057/webrev.04/
[2] http://cr.openjdk.java.net/~jjiang/8186057/webrev.04/test/jdk/javax/net/ssl/compatibility/README.html

Best regards,
John Jiang

On 07/09/2017 18:47, sha.ji...@oracle.com wrote:
Hi Artem,

On 07/09/2017 16:28, Artem Smotrakov wrote:
In case of SNI, SSLParemeters.setServerNames() method (and others related to SNI) was introduced in 8. I don't think the code which use this method can be compiled with 6 and 7.
All of Java source are compiled by a JDK 10 build, which is specified by jtreg option "-jdk". Other testing JDKs, including 6 and 7, just use the .class files on runtime.
That's why the below line exists in Compatibility.java,
32  * @compile -source 1.6 -target 1.6 Server.java Client.java JdkUtils.java

But if you want to test SNI with 8+, you need to call them.

https://docs.oracle.com/javase/8/docs/api/javax/net/ssl/SSLParameters.html#setServerNames-java.util.List-
If a feature, like ALPN and SNI, is not supported, such method would not be called on runtime.

Best regards,
John Jiang

Artem

On 09/07/2017 11:24 AM, sha.ji...@oracle.com wrote:
Hi Artem,

On 07/09/2017 16:07, Artem Smotrakov wrote:

- Please use try-with-resources if possible (files, sockets, etc)
The test uses only JDK 6-supported language features, but try-with-resources is introduced by JDK 7.
Here we come to another issue. I believe that it would be good to use write clients for all JDK versions. If you use the same code for all Java versions, that means that you use only JDK 6 API. Let's consider the following:
- we want to test 8 vs 9
- 8 and 9 may have some API (or just functionality) which is not supported by 6 (for example, ALPN, SNI)

If you write code which is compatible with 6, then you can use API from newer versions, but we still may want to test it.
In fact, the current test has already covered ALPN, though only JDK 9 and 10 support the associated methods, like SSLParameters.getApplicationProtocols(). ALPN-associated case combinations are ignored if a JDK doesn't support this feature. Exactly, JdkUtils checks if a specific JDK build contains method SSLParameters.getApplicationProtocols().
I think the same approach could be applied for SNI in the future.

Best regards,
John Jiang

Artem

Best regards,
John Jiang

Artem

On 09/07/2017 08:52 AM, sha.ji...@oracle.com wrote:
Hi,
Please review this test for checking the interop compatibility on JSSE among different JDK releases (from 6 to 10). It covers the cases, like handshake, data exchange, client authentication and APLN, on all TLS versions (if possible). And the selected TLS cipher suites are: TLS_RSA_WITH_AES_128_CBC_SHA, TLS_DHE_DSS_WITH_AES_128_CBC_SHA and TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA.
For more details, please look though the README.

Webrev: http://cr.openjdk.java.net/~jjiang/8186057/webrev.00
Issue: https://bugs.openjdk.java.net/browse/JDK-8186057

Best regards,
John Jiang













Reply via email to