[9] RFR: 8168969: Merge SSLSocketSample and SSLSocketTemplate

[Xuelei Fan]

I don't like it, but as the 1st stage of the improvement, it looks fine 
to me.

Xuelei

On 11/16/2016 10:52 AM, Xuelei Fan wrote:
> 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
>>>>>>
>>>>>
>>>
>>