Hi Artem,
On 2016/11/2 23:54, Artem Smotrakov wrote:
Hi John,
Please see inline.
On 11/01/2016 11: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.
Correct, AnonCipherWithWantClientAuth can work fine without extending
SSLSocketTemplate.
I make those classes to extend SSLSocketTemplate to make lines shorter
(we keep lines shorter than 80 symbols).
In practice, you could divide one line to two lines if the line length
is greater than 80.
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.
I think it may be better to keep them static which also means
stateless here. It would be easier to re-use in my opinion.
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).
I see your point, and agree with that. I would prefer to add this
extension when we update a test which really needs it. If you have
such a bug, please feel free to do that.
The test AnonCipherWithWantClientAuth.java in your patch looks need this
extension.
We have more than one hundred SSL/TLS tests, and they may need some
other extensions. I am not sure that it's possible to provide a
universal SSLSocketTemplate which fits all test's needs.
I see. It's unnecessary to design too much before getting more concrete
requirements.
Best regards,
John Jiang
Artem
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
