Hi Siba Change looks fine. I have no other comments.
Thanks Max > On Aug 23, 2017, at 8:30 PM, Sibabrata Sahoo <[email protected]> > 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; [email protected] > 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 <[email protected]> >> 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; [email protected] >> 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 <[email protected]> >>> 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; [email protected] >>> 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 <[email protected]> >>>> 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 >>> >> >
