[8u] TLSv1.3 RFR: 8251478: Backport TLSv1.3 regression tests to JDK8u

[Alexey Bakhtin]

Hi Martin,

Please find my comments below.

New webrev: http://cr.openjdk.java.net/~abakhtin/tls1.3/8245466/8251478/webrev.v2
Git diff:  http://cr.openjdk.java.net/~abakhtin/tls1.3/8245466/8251478/webrev.v2/jdk.git.diff

Regards
Alexey

> On 24 Aug 2020, at 19:20, Martin Balao <mbalao at redhat.com> wrote:
> 
> Hi Alexey,
> 
> On 8/19/20 10:44 AM, Alexey Bakhtin wrote:
>> Please find updated version of the patch:
>> Webrev: http://cr.openjdk.java.net/~abakhtin/tls1.3/8245466/8251478/webrev.v1
>> Git diff:  http://cr.openjdk.java.net/~abakhtin/tls1.3/8245466/8251478/webrev.v1/jdk.git.diff
>> 
>> This version contains updates for the MFLN Extension tests
> 
> Thanks for proposing this patch.
> 
> A few questions:
> 
> * test/sun/security/ssl/SSLSocketImpl/SSLSocketKeyLimit.java
>  * 8u's equivalent of InputStream::readNBytes is IOUtils.readNBytes
OK. Fixed. But actually, it does not matter How many bytes
read from the stream  in this particular case.
>  * Just out of curiosity, why 'main' method needs to throw Throwable
> instead of Exception? I've seen this change in other tests too.
In JDK8 jdk.testlibrary.ProcessTools.executeProcess() throws Throwable instead of Exception. This is a reason of this fix.
> 
> * test/sun/security/ssl/internal/java.base/sun/security/ssl/TestHkdf.java
> *
> test/sun/security/ssl/Stapling/java.base/sun/security/ssl/StatusResponseManagerTests.java
>  * Why did you relocate these tests? I'd keep them in their original
> location if possible.
It was part of task to exclude modularisation dependencies. But you are right. It is not necessary. Reverted.
> 
> * test/sun/security/ssl/X509TrustManagerImpl/Symantec/Distrust.java
>  * I've seen the jdk.test.lib.security.SecurityUtils import was removed
> but looks to me that it's used. I might be overlooking something.
> 
JDK8 already has SecurityUtils.java class in the test/lib/security directory. This class is in the unnamed package. So, Distrust.java is updated to use original SecurityUtils.java class

> * MFLNTest tests
>  * I'd try something different, to minimize the number of changes.
> Instead of having scripts that build a JAR and include the new
> MaxPacketSize class in the sun.security.ssl package to access internal
> fields; I'd only change SSLEngineTestCase::doHandshake method to set the
> proper max packet size by means of reflection.
> SSLEngineTestCase::doHandshake has to be edit anyways to reflect the
> lack of an API to set the max fragment size. This approach should also
> avoid a change in MFLNTest class. What do you think?

Yes, you are right. Thank you. These tests could be simplified by using reflection. Fixed.

> 
> Thanks,
> Martin.-
>