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@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.sa...@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@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.sa...@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 >
