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

classic Classic list List threaded Threaded
9 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

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 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; [hidden email]
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 <[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

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
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 <[hidden email]> 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; [hidden email]
> 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 <[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
>

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 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; [hidden email]
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 <[hidden email]> 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; [hidden email]
> 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 <[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
>

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
Then maybe you can also make setup() a separate @run.

--Max

> On Aug 23, 2017, at 4:17 PM, Sibabrata Sahoo <[hidden email]> 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; [hidden email]
> 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 <[hidden email]> 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; [hidden email]
>> 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 <[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
>>
>

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 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; [hidden email]
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 <[hidden email]> 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; [hidden email]
> 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 <[hidden email]> 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; [hidden email]
>> 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 <[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
>>
>

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

Change looks fine. I have no other comments.

Thanks
Max

> On Aug 23, 2017, at 8:30 PM, Sibabrata Sahoo <[hidden email]> 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; [hidden email]
> 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 <[hidden email]> 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; [hidden email]
>> 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 <[hidden email]> 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; [hidden email]
>>> 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 <[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...