[9] RFR: 8151654: Additional modular test for "auth.login.defaultCallbackHandler" property

[Weijun Wang]

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 at 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.sahoo at 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
>