[10] RFR: JDK-8183310: java/security/modules/ModularTest.java should clean up better
Sibabrata Sahoo
sibabrata.sahoo at oracle.com
Tue Aug 22 13:36:16 UTC 2017
Hi Max,
Please find the updated webrev addressing all comments in applicable test files.
Webrev: http://cr.openjdk.java.net/~ssahoo/8183310/webrev.01/
Regarding "172-180: This sounds like you can use legacy jars as module jars and vice versa. Is this something new?"
This is just additional Test case for regular jars in modulepath and modular jars in classpath, which verifies how the type get resolved.
Thanks,
Siba
-----Original Message-----
From: Weijun Wang
Sent: Wednesday, August 02, 2017 3:53 PM
To: Sibabrata Sahoo
Cc: Valerie Peng; security-dev at openjdk.java.net
Subject: Re: [10] RFR: JDK-8183310: java/security/modules/ModularTest.java should clean up better
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 <sibabrata.sahoo at oracle.com> 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
More information about the security-dev
mailing list