I don't like it, too. It is not necessary to make the merge in a hurry.
Maybe, we can wait for a while so that we have time for a real and
comfortable template.
Thanks,
Xuelei
On 11/16/2016 10:44 AM, Artem Smotrakov wrote:
I don't like it, but no lambdas, no nothing in SSLSocketTemplate.java
http://cr.openjdk.java.net/~asmotrak/8168969/webrev.01/
Xuelei,
Could you please take a look?
Artem
On 11/02/2016 09:14 AM, Artem Smotrakov wrote:
Hi Xuelei,
This totally makes sense. But in my opinion we should use new features
and APIs because:
- they are here to make code readable (okay, not to much lambdas)
- they help to avoid bugs (e.g., forgetting to close resources)
- we implicitly provide test coverage for new APIs
- we give examples how these features can be used
- finally, we encourage people to move to new Java versions
Not sure if I got correction what you mean by removing the part to
declare the store files in each sub-classes. Do you mean getting rid
of setting keystore/truststore in actual test files?
If so, I would prefer to do it in a separate enhancement. Most of
SSL/TLS tests use keystores from "etc" directory. The test use
"test.src" system property (set by jtreg) to build a path to "etc",
and it depends on actual directory where a test stays. Also if I
recall correctly, some tests use their own keystore. This update may
be quite big. Would you mind if we did it separately?
8168969 is about merging helper classes to remove possible confusions
Brad mentioned recently.
Artem
On 11/02/2016 03:57 AM, Xuelei Fan wrote:
Please use JDK 6 only features (no Lambda, try-with-resource,
multi-catch, java.util.Objects, etc.) so as to expedite porting to
previous releases.
It would be nice if removing the part to declare the store files in
each sub-classes.
Xuelei
On 11/2/2016 2:48 PM, John Jiang wrote:
Hi Artem,
Thanks for making the template to be used easily.
The tests in your patch extend class SSLSocketTemplate, but
SSLSocketTemplate looks like an utility class, but not a parent class.
For example,
test/sun/security/ssl/ServerHandshaker/AnonCipherWithWantClientAuth.java
67 new SSLSocketTemplate()
AnonCipherWithWantClientAuth still creates a SSLSocketTemplate
instance,
but not itself.
In fact, if replacing "print()" with "SSLSocketTemplate.print()",
AnonCipherWithWantClientAuth can work fine without extending
SSLSocketTemplate.
The original SSLSocketSample.java defines the core methods, such as
doServerSide(), runServerApplication(), doClientSide() and
runClientApplication(), as non-static. In my eyes, this style may be
better. The child class can simply override the methods if necessary.
In addition, some tests have to configure SSLServerSocket. Why not
provide one more extension point in doServerSide()? Then, it
unnecessary
to re-write the whole doServerSide() (or, set a new server peer).
The code talks more clearly. Please take a look at my example:
http://cr.openjdk.java.net/~jjiang/8168969/example/
Best regards,
John Jiang
On 2016/11/2 8:54, Artem Smotrakov wrote:
Hello,
Please review the following patch which merges a couple of classes in
javax/net/ssl/templates.
SSLTest class contains re-usable parts of SSLSocketSample.
SSLSocketTemplate class is buggy (tests which follows it may fail
intermittently). I basically replaced SSLSocketTemplate with SSLTest,
and removed SSLSocketSample.
SSL/TLS tests should use SSLSocketTemplate class. I updated test which
use SSLTest to use SSLSocketTemplate.
Bug: https://bugs.openjdk.java.net/browse/JDK-8168969
Webrev: http://cr.openjdk.java.net/~asmotrak/8168969/webrev.00/
Artem
