On 7/22/2016 17:12, Sibabrata Sahoo wrote:
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.
Well, you have S_BUILD_DIR and BUILD_DIR and then suddently C_BLD_DIR.
Just looks strange.
- 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.
No need to worry about this. I believe if a system property is defined
multiple times, the last one wins out.
- 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.
The arguments are all in a comma-separated-list, why is List<List>
better looking?
- 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.
Sounds reasonable.
Anyway, all my comments are about styles and not correctness. Feel free
to ignore them.
Thanks
Max
Thanks,
Siba
-----Original Message-----
From: Wang Weijun
Sent: Sunday, July 17, 2016 1:12 PM
To: Sibabrata Sahoo
Cc: security-dev@openjdk.java.net; 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 <sibabrata.sa...@oracle.com> 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
