[10] RFR: JDK-8183310: java/security/modules/ModularTest.java should clean up better

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[10] RFR: JDK-8183310: java/security/modules/ModularTest.java should clean up better

Sibabrata Sahoo

Hi,

 

Please review the patch for the following,

 

JBS: https://bugs.openjdk.java.net/browse/JDK-8183310

Webrev: http://cr.openjdk.java.net/~ssahoo/8183310/webrev.00/

 

Change description:

The code has been changed significantly for cleaning up existing code and to simplify it. During clean up, I have also removed the unwanted file “java/security/modules/ModularTest.java”.

 

SecurityProviderModularTest.java - Verify security provider in all possible modular combination. There are 2 related files “TestClient.java” and “TestProvider.java” renamed and changed a little during clean up.

JaasModularClientTest.java – Verify JAAS login module in all possible modular combination.

JaasModularDefaultHandlerTest.java - Verify JAAS default handler in all possible modular combination. As part of clean I have also removed “TEST.properties” file.

 

Thanks,

Siba

 

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RE: [10] RFR: JDK-8183310: java/security/modules/ModularTest.java should clean up better

Sibabrata Sahoo

Hi,

 

This patch is pending for your review.

 

Thanks,

Siba

 

From: Sibabrata Sahoo
Sent: Saturday, July 15, 2017 4:00 PM
To: Weijun Wang; Valerie Peng; [hidden email]
Subject: [10] RFR: JDK-8183310: java/security/modules/ModularTest.java should clean up better

 

Hi,

 

Please review the patch for the following,

 

JBS: https://bugs.openjdk.java.net/browse/JDK-8183310

Webrev: http://cr.openjdk.java.net/~ssahoo/8183310/webrev.00/

 

Change description:

The code has been changed significantly for cleaning up existing code and to simplify it. During clean up, I have also removed the unwanted file “java/security/modules/ModularTest.java”.

 

SecurityProviderModularTest.java - Verify security provider in all possible modular combination. There are 2 related files “TestClient.java” and “TestProvider.java” renamed and changed a little during clean up.

JaasModularClientTest.java – Verify JAAS login module in all possible modular combination.

JaasModularDefaultHandlerTest.java - Verify JAAS default handler in all possible modular combination. As part of clean I have also removed “TEST.properties” file.

 

Thanks,

Siba

 

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] RFR: JDK-8183310: java/security/modules/ModularTest.java should clean up better

Weijun Wang
In reply to this post by Sibabrata Sahoo
I am reading JaasModularClientTest.java now. It’s much simpler than the original one. Some comments:

38: This ProcessTools is deprecated. Is it possible to use the one in root repo?

53: Why make it public?

98-99: What localized strings are you trying to avoid?

103-104: Why do these 2 variables contain the option name? I mean in the following expression

    String.format("-cp %s --module-path %s %s %s", unnC, modL, addNMArg, C_TYPE)

you have put some option names (-cp, --module-path) in the format string but another (--add-modules) in an argument (addNMArg). It will be more clear to put option names in format string and option values in arguments, say

    String.format("-cp %s --module-path %s --add-modules %s %s", unnC, modL, "ml", C_TYPE)

Or just simply

    String.format("-cp %s --module-path %s --add-modules ml %s", unnC, modL, C_TYPE)


128: This loop looks a little strange. I'd rather just write the content twice.

136: This is the only usage of service parameter? If so, why not just move the line into constructor?

140: Why is there a "-" after "Case:"?

141: In every case you are expecting EXP_RES. Why bother include it as a parameter? In fact, what else do you expect? If EXP_RES is not seen, shouldn't the client just fails with an exception? If so, why check contains() on line 197 and not look at the exit value?

172-180: This sounds like you can use legacy jars as module jars and vice versa. Is this something new?

212-245: Please add an empty line before every new mBuilder assignment.

221: So you need jdk.security.auth for UnixPrincipal? It looks like this class is also available on Windows but I feel it a little strange. Maybe use UserPrincipal or create your new one?

I'll read the other two tests and hopefully they have similar structures.

Thanks
Max

> On Jul 15, 2017, at 6:30 PM, Sibabrata Sahoo <[hidden email]> wrote:
>
> Hi,
>  
> Please review the patch for the following,
>  
> JBS: https://bugs.openjdk.java.net/browse/JDK-8183310
> Webrev: http://cr.openjdk.java.net/~ssahoo/8183310/webrev.00/
>  
> Change description:
> The code has been changed significantly for cleaning up existing code and to simplify it. During clean up, I have also removed the unwanted file “java/security/modules/ModularTest.java”.
>  
> SecurityProviderModularTest.java - Verify security provider in all possible modular combination. There are 2 related files “TestClient.java” and “TestProvider.java” renamed and changed a little during clean up.
> JaasModularClientTest.java – Verify JAAS login module in all possible modular combination.
> JaasModularDefaultHandlerTest.java - Verify JAAS default handler in all possible modular combination. As part of clean I have also removed “TEST.properties” file.
>  
> Thanks,
> Siba

Loading...