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

Weijun Wang weijun.wang at oracle.com
Wed Aug 23 13:43:03 UTC 2017


Hi Siba

Change looks fine. I have no other comments.

Thanks
Max

> On Aug 23, 2017, at 8:30 PM, Sibabrata Sahoo <sibabrata.sahoo at oracle.com> wrote:
> 
> Hi Max,
> 
> Please find the updated webrev: http://cr.openjdk.java.net/~ssahoo/8183310/webrev.02/
> 
> The setup will be called only once irrespective number of @run available inside the Test file.
> 
> Thanks,
> Siba
> 
> -----Original Message-----
> From: Weijun Wang 
> Sent: Wednesday, August 23, 2017 3:04 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
> 
> Then maybe you can also make setup() a separate @run.
> 
> --Max
> 
>> On Aug 23, 2017, at 4:17 PM, Sibabrata Sahoo <sibabrata.sahoo at oracle.com> wrote:
>> 
>> Hi Max,
>> 
>> I have splitted the @run to convert each run to consume minimum time to avoid any timeout for rare cases.
>> 
>> Thanks,
>> Siba
>> 
>> -----Original Message-----
>> From: Weijun Wang 
>> Sent: Wednesday, August 23, 2017 1:43 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
>> 
>> Is there any reason why service being "true" and "false" are in 2 @run? This means setup() will run twice.
>> 
>> --Max
>> 
>>> On Aug 22, 2017, at 9:36 PM, Sibabrata Sahoo <sibabrata.sahoo at oracle.com> wrote:
>>> 
>>> 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