[8u] TLSv1.3 RFR: 8245477: Adjust TLS tests location

[Alexey Bakhtin]

Hello Martin,

Please find my answers below

Regards
Alexey

> On 25 Jul 2020, at 01:20, Martin Balao <mbalao at redhat.com> wrote:
> 
> Hi Alexey,
> 
> On Wed, Jul 22, 2020 at 11:55 AM Alexey Bakhtin <alexey at azul.com> wrote:
>> 
>> Please find updated patch for JDK-8245477: http://cr.openjdk.java.net/~abakhtin/tls1.3/8245466/8245477/webrev.v1/
>> 
> 
> Thanks for submitting the series of patches related to TLS tests. I'm
> trying to understand the rationale behind each step and have some
> questions.
> 
> 1) Those tests relocated as part of Step 9 - 8245477, are 8u tests or
> did you replace them with newer JDK-11 versions in a single step?

Step 9 relocates 8u test only. It does not replace tests with JDK-11 version
> 
> 2) Is there any other change in addition to relocation and 'references
> to keystore and template files' modifications? My understanding is
> that they are just JDK-8 relocated tests with some fixes only to pass,
> but there is nothing new on them.

Yes, you are correct.  The only changes in the tests is update path to keystore and template files.

> 
> 3) Should we expect all tests passing in Step 9? If not, why? I read
> 'no regression' in your first comment but that does not say whether
> they are all passing or not.

I can not say all test passed, because of three test fails on the original TLSv1.2 implementation
and original test file location :
- javax/net/ssl/ciphersuites/DisabledAlgorithms.java
- sun/security/ssl/com/sun/net/ssl/internal/ssl/X509KeyManager/PreferredKey.java
- sun/security/ssl/sanity/interop/ClientJSSEServerJSSE.java
These test still fails after relocation with original TLSv1.2 implementation.
This is a reason I wrote “no regression” instead of all test passed.

> 
> 4) I've seen tests that were relocated in Step 9 (and fixed), deleted
> in Step 10 and added as new files in Step 11 (i.e.
> RejectClientRenego.java). How did you decide which tests had to be
> relocated, which had to be deleted in Step 10 and which had to be
> added in Step 11?

In Step 9 I have compared test location in JDK8 and JDK11 and perform test relocation.
So, after relocation tests located in the same path in JDK8 and JDK11
It will simplify father test merge, because of test located in the same test path
E.g.with RejectClientRenego.java test:
1) RejectClientRenego.java is located in
sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLSocketImpl in JDK8 repository
sun/security/ssl/SSLSocketImpl in JDK11 repository
In Step9 RejectClientRenego.java is moved to test/sun/security/ssl/SSLSocketImpl directory
2) RejectClientRenego.java is not identical in the JDK8 and JDK11 repositories.
So, RejectClientRenego.java is removed in Step 10.
3) New version of RejectClientRenego.java is added at Step 11.

> 
> 5) Was any test not relocated in Step 9, not deleted in Step 10 or not
> added in Step 11? Why?

Yes. There are a lot of test that was not modified in terms of location or implementation. These test was not modified in Step 9, Step 10 or Step 11:
E.g.:
- sun/security/ssl/CertPathRestrictions/JSSEClient.java
- sun/security/ssl/CipherSuite/NoDesRC4CiphSuite.java
- sun/security/ssl/DHKeyExchange/LegacyDHEKeyExchange.java
- sun/security/ssl/HandshakeHash/*
- sun/security/ssl/ServerHandshaker/HelloExtensionsTest.java
- sun/security/ssl/SSLContextImpl/CustomizedCipherSuites.java
and many other tests

> 
> 6) What would be the downside if we just delete
> test/sun/security/ssl/* (and any other directory whose tests are
> SSL-related) and in the next step add them from JDK-11?

As mentioned above, A lot of test was not relocated or modified.
So, in this case we just lost history without good reason for unmodified tests.
Another reason - is JDK8 contains extra tests which is not present in JDK11:

- sun/security/ssl/X509KeyManager/CertificateAuthorities.java
- sun/security/ssl/SSLEngineImpl/CloseInboundException.java
- sun/security/ssl/SSLContextImpl/DefautlCacheSize.java
- sun/security/ssl/SessionIdCollisionTest.java
- sun/net/www/https/ChunkedOutputStream/Test.java (was sun/security/ssl/sun/net/www/http/ChunkedOutputStream/Test.java)

If we go with delete/add solution we could lost these tests.

Also, my solution with three steps allow to verify test consistence after each step :
- after Step 9 - no regression with original TLSv1.2 implementation
- after Step 10 - test passed with original TLSv1.2 implementation and new TLSv1.3 implementation
- after Step 11 - test passed with new TLSv1.3 implementation

I understand your concerns about review, but my suggestion keeps more history and allows to track test relocation.

> 
> The problem here, from a reviewer's side, is that I need a criteria to
> judge these test relocations. This criteria would be the 8u backport
> of 8032473, but I'm not sure that we are aligned on that. 8032473
> won't apply cleanly and it's a mega-patch. If we are doing relocations
> to proceed with file replacement later (by file replacement I mean
> deleting everything in one step and adding all in the next), we can
> simplify things and do file replacement now agreeing on which are the
> SSL-related directories. In example: why not deleting
> "sun/security/ssl" and adding (from JDK-11) "sun/security/ssl",
> "sun/net/www/protocol/https", "javax/net/ssl" and "com/sun/net/ssl"?
> Note: there can be tests that fail because they expect a different
> public API. But that is unavoidable.
> 
> NOTE: I'm just brainstorming, not convinced yet of anything.
> 
> Thanks,
> Martin.-