Hi Max, Please find the update webrev: http://cr.openjdk.java.net/~ssahoo/8151654/webrev.01/
Here is the change as per comment, - I renamed C_BUILD_DIR to C_BLD_DIR just make few line to put within 80 character space. I think it should be fine here. - As suggested, "-addmods m" option added to vmArgs parameter of the getJavaCommand() method. Making vmArgs be a map makes more sense for holding JVM parameters(name, value) as well as makes sure the properties are not defined twice. - I understand that getTestInput() can directly return an array of Object[][]. But still I used List<List> with intension. The reason is, I found it make the test input look more cleaner and it makes the line easily readable than an array. It got additional code but I think we still can have it from the point where it make more readable. After all it does not have any reasonable performance impact. - I did few change to existing Test to add "-addmods". It is useful for the case where a transitive dependency need to be added to root module explicitly. This is not required for the case where the modular graph is much clearly defined through module descriptor. This might require in certain cases where the module need to be linked to the root module in explicit way. Regarding the change to existing test, I feel still it is optional but good to have. The reason is, either we add a module explicitly or not, still it is a valid test case. Thanks, Siba -----Original Message----- From: Wang Weijun Sent: Sunday, July 17, 2016 1:12 PM To: Sibabrata Sahoo Cc: [email protected]; Mandy Chung; Valerie Peng Subject: Re: [9] RFR: 8151654: Additional modular test for "auth.login.defaultCallbackHandler" property Why change C_BUILD_DIR to C_BLD_DIR everywhere? Can you add the "-addmods m" option into the vmArgs parameter of the getJavaCommand() method? In fact, why must vmArgs be a map instead of a List<String>? Are you worried about the same system property be assigned twice? There is no need to generate lists and convert it to array in getTestOutput(), just return what you need, like this: return new Object[][] { {1,2,3}, {4,5,6}, }; BTW, what's the main reason you rewrite existing tests? Thanks Max > On Jul 15, 2016, at 1:01 AM, Sibabrata Sahoo <[email protected]> > wrote: > > Hi, > > Please review the following patch for “Additional modular test for > "auth.login.defaultCallbackHandler" property”. > > JBS: https://bugs.openjdk.java.net/browse/JDK-8151654 > Webrev: http://cr.openjdk.java.net/~ssahoo/8151654/webrev.00/ > > Description: > A custom callback handler for JAAS can be provided through > "auth.login.defaultCallbackHandler" security property. This patch provides > test the behavior when the callback handler available in module/class path as > modular/regular jar file. > There are few minor changes over few existing test files due to missing > “-addmods” during runtime for automated module dependency. > > Thanks, > Siba
