[9] RFR: 8168969: Merge SSLSocketSample and SSLSocketTemplate

[John Jiang]

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
>>>
>>
>
>