Re: RFR: 8158670: Fix @modules in java/lang/SecurityManager/CheckSecurityProvider.java

[Valerie Peng]

Ok, please send me your putback comment. Otherwise, I will just use my 
own wording. ;)

Should get it done either later today or tomorrow...
Thanks,
Valerie

On 7/6/2016 11:31 AM, Alexandre (Shura) Iline wrote:

Valerie, could you sponsor the patch for me?

Shura


On Jul 6, 2016, at 10:08 AM, Valerie Peng  wrote:


Changes look fine to me.
Thanks,
Valerie

On 7/5/2016 2:31 PM, Mandy Chung wrote:

On Jul 5, 2016, at 1:53 PM, Alexandre (Shura) Iline 
 wrote:



On Jul 5, 2016, at 1:36 PM, Mandy Chung  wrote:



On Jul 5, 2016, at 12:42 PM, Alexandre (Shura) Iline 
 wrote:

This made sense, than you, Mandy.

Please review new version:
http://cr.openjdk.java.net/~shurailine/8158670/webrev.02/

You can use Layer::findModule instead of Configuration::findModule.

That is correct. Changed in place.


You can also use List::equals.

I am assuming you are suggesting to use List::equals in the bottom part of the 
test where the expected result is compared with the actual list of providers. 
The whole reason I redid that section to provide more information in the jtr 
file, both for a case of a failure and to find out what providers were actually 
expected for given configuration. I do not see how List:equals help me with 
that. Information on size mismatch is useful, and also the information on 
unexpected provider name.

What you have is fine.  The information is useful to help diagnosis.  The 
alternative I was thinking is to check List::equals and if not equals, do 
line-108-117.  It’s a minor thing and up to you.

Mandy

Re: RFR: 8158670: Fix @modules in java/lang/SecurityManager/CheckSecurityProvider.java

[Alexandre (Shura) Iline]

Valerie, could you sponsor the patch for me?

Shura

> On Jul 6, 2016, at 10:08 AM, Valerie Peng  wrote:
> 
> 
> Changes look fine to me.
> Thanks,
> Valerie
> 
> On 7/5/2016 2:31 PM, Mandy Chung wrote:
>>> On Jul 5, 2016, at 1:53 PM, Alexandre (Shura) Iline 
>>>  wrote:
>>> 
>>> 
 On Jul 5, 2016, at 1:36 PM, Mandy Chung  wrote:
 
 
> On Jul 5, 2016, at 12:42 PM, Alexandre (Shura) Iline 
>  wrote:
> 
> This made sense, than you, Mandy.
> 
> Please review new version:
> http://cr.openjdk.java.net/~shurailine/8158670/webrev.02/
 You can use Layer::findModule instead of Configuration::findModule.
>>> That is correct. Changed in place.
>>> 
 You can also use List::equals.
>>> I am assuming you are suggesting to use List::equals in the bottom part of 
>>> the test where the expected result is compared with the actual list of 
>>> providers. The whole reason I redid that section to provide more 
>>> information in the jtr file, both for a case of a failure and to find out 
>>> what providers were actually expected for given configuration. I do not see 
>>> how List:equals help me with that. Information on size mismatch is useful, 
>>> and also the information on unexpected provider name.
>> What you have is fine.  The information is useful to help diagnosis.  The 
>> alternative I was thinking is to check List::equals and if not equals, do 
>> line-108-117.  It’s a minor thing and up to you.
>> 
>> Mandy
> 

Re: RFR: 8158670: Fix @modules in java/lang/SecurityManager/CheckSecurityProvider.java

[Valerie Peng]

Changes look fine to me.
Thanks,
Valerie

On 7/5/2016 2:31 PM, Mandy Chung wrote:

On Jul 5, 2016, at 1:53 PM, Alexandre (Shura) Iline 
 wrote:



On Jul 5, 2016, at 1:36 PM, Mandy Chung  wrote:



On Jul 5, 2016, at 12:42 PM, Alexandre (Shura) Iline 
 wrote:

This made sense, than you, Mandy.

Please review new version:
http://cr.openjdk.java.net/~shurailine/8158670/webrev.02/

You can use Layer::findModule instead of Configuration::findModule.

That is correct. Changed in place.


You can also use List::equals.

I am assuming you are suggesting to use List::equals in the bottom part of the 
test where the expected result is compared with the actual list of providers. 
The whole reason I redid that section to provide more information in the jtr 
file, both for a case of a failure and to find out what providers were actually 
expected for given configuration. I do not see how List:equals help me with 
that. Information on size mismatch is useful, and also the information on 
unexpected provider name.

What you have is fine.  The information is useful to help diagnosis.  The 
alternative I was thinking is to check List::equals and if not equals, do 
line-108-117.  It’s a minor thing and up to you.

Mandy

Re: RFR: 8158670: Fix @modules in java/lang/SecurityManager/CheckSecurityProvider.java

[Mandy Chung]

> On Jul 5, 2016, at 1:53 PM, Alexandre (Shura) Iline 
>  wrote:
> 
> 
>> On Jul 5, 2016, at 1:36 PM, Mandy Chung  wrote:
>> 
>> 
>>> On Jul 5, 2016, at 12:42 PM, Alexandre (Shura) Iline 
>>>  wrote:
>>> 
>>> This made sense, than you, Mandy.
>>> 
>>> Please review new version:
>>> http://cr.openjdk.java.net/~shurailine/8158670/webrev.02/
>> 
>> You can use Layer::findModule instead of Configuration::findModule.
> 
> That is correct. Changed in place.
> 
>> 
>> You can also use List::equals.
> 
> I am assuming you are suggesting to use List::equals in the bottom part of 
> the test where the expected result is compared with the actual list of 
> providers. The whole reason I redid that section to provide more information 
> in the jtr file, both for a case of a failure and to find out what providers 
> were actually expected for given configuration. I do not see how List:equals 
> help me with that. Information on size mismatch is useful, and also the 
> information on unexpected provider name.

What you have is fine.  The information is useful to help diagnosis.  The 
alternative I was thinking is to check List::equals and if not equals, do 
line-108-117.  It’s a minor thing and up to you.

Mandy

Re: RFR: 8158670: Fix @modules in java/lang/SecurityManager/CheckSecurityProvider.java

[Alexandre (Shura) Iline]

> On Jul 5, 2016, at 1:36 PM, Mandy Chung  wrote:
> 
> 
>> On Jul 5, 2016, at 12:42 PM, Alexandre (Shura) Iline 
>>  wrote:
>> 
>> This made sense, than you, Mandy.
>> 
>> Please review new version:
>> http://cr.openjdk.java.net/~shurailine/8158670/webrev.02/
> 
> You can use Layer::findModule instead of Configuration::findModule.

That is correct. Changed in place.

> 
> You can also use List::equals.

I am assuming you are suggesting to use List::equals in the bottom part of the 
test where the expected result is compared with the actual list of providers. 
The whole reason I redid that section to provide more information in the jtr 
file, both for a case of a failure and to find out what providers were actually 
expected for given configuration. I do not see how List:equals help me with 
that. Information on size mismatch is useful, and also the information on 
unexpected provider name.

Shura 

> 
> Mandy

Re: RFR: 8158670: Fix @modules in java/lang/SecurityManager/CheckSecurityProvider.java

[Mandy Chung]

> On Jul 5, 2016, at 12:42 PM, Alexandre (Shura) Iline 
>  wrote:
> 
> This made sense, than you, Mandy.
> 
> Please review new version:
> http://cr.openjdk.java.net/~shurailine/8158670/webrev.02/

You can use Layer::findModule instead of Configuration::findModule.

You can also use List::equals.

Mandy

Re: RFR: 8158670: Fix @modules in java/lang/SecurityManager/CheckSecurityProvider.java

[Alexandre (Shura) Iline]

Jon,

> On Jul 1, 2016, at 6:44 PM, Jonathan Gibbons  
> wrote:
> 
> The test can also report which providers it is testing and/or skipping, so 
> that anyone checking the .jtr file can verify the behavior.

I have added some debug output, pls take a look.
http://cr.openjdk.java.net/~shurailine/8158670/webrev.02/

> 
> Maybe it fails if expected modules or providers are not found.

In the current state the test would work with any module availabilities - it 
simply checks that order and presence of security providers is consistent with 
the module availabilities.

Shura

> 
> -- Jon
> 
> 
> On 06/29/2016 10:50 AM, Mandy Chung wrote:
>> Valerie’s suggestion is a good one.  The test will require a minimum image 
>> with cross-platform security providers to run the test while it can still 
>> verify other platform-specific providers.
>> 
>> Mandy
>> 
>>> On Jun 29, 2016, at 10:41 AM, Valerie Peng  wrote:
>>> 
>>> 
>>> It's not like the test silently passes as the test still covers the 
>>> cross-platform modules.
>>> The way I view this is that the platform=specific modules are "optional" 
>>> and we update the expected result by detecting their presence (or the not). 
>>> It's not a hack or workaround, but rather an enhancement for the test to 
>>> handle different images.
>>> 
>>> Just my .02,
>>> Valerie
>>> 
>>> On 6/29/2016 10:22 AM, Alexandre (Shura) Iline wrote:
> On Jun 28, 2016, at 5:22 PM, Valerie Peng  wrote:
> 
> 
> One of the purpose of this test is to test the ordering (see the initial 
> bug which this test is for: JDK-6997010).
> 
> The original test already detects the OS and will skip certain providers 
> accordingly.
> Instead of splitting the test into multiple platform-specific tests, 
> maybe we can keep the original test but add module-presence checking? Is 
> there API available to query if certain module are present?
 ModuleFinder.ofSystem().find(String).
 
 We can have only the cross-platform modules listed in @modules and make 
 the test to pass silently if the required platform-specific modules are 
 not present.
 
 So, for example, on windows, if the test would be executed against an 
 image which have no jdk.crypto.mscapi, the test will not run any checks 
 and report pass.
 
 This would help to avoid the additional test creation, but it will add 
 another silently passing test, which is less clean.
 
 Mandy?
 
 Shura
 
> If yes, then we can leave out the platform-specific providers from the 
> @modules line and skip the providers if either the OS does not match or 
> the module is not present.
> 
> If we can't query what modules are available, then we may have to think 
> of something else.
> Valerie
> 
> On 6/27/2016 12:27 PM, Mandy Chung wrote:
>> I’m including security-dev which would be a better list to review this 
>> test fix.
>> 
>> Valerie,
>>Does this test have to be order-sensitive?  I think this test would 
>> be cleaner to make it order-insensitive and simply test the security 
>> provider initialization.
>> 
>> See my comments below.
>> 
>>> On Jun 27, 2016, at 8:21 AM, Alexandre (Shura) Iline 
>>>  wrote:
>>> 
>>> Hi.
>>> 
>>> Please take a look on a suggested for for the 
>>> java/lang/SecurityManager/CheckSecurityProvider.java test.
>>> 
>>> The test in question depend on a list of modules, some of them are 
>>> platform-specific. Listing all the dependencies in one test is causing 
>>> the test to be skipped on every platform. In an offline conversation it 
>>> was decided that it is better to split this tests into a few tests to 
>>> declare the per-platform module dependencies.
>>> 
>>> The bug: https://bugs.openjdk.java.net/browse/JDK-8158670
>>> The suggested fix: 
>>> http://cr.openjdk.java.net/~shurailine/8158670/webrev.00/
>> The copyright header start year of the new tests should be 2016.
>> 
>> I would suggest to make CheckSecurityProvide a platform-neutral test, 
>> i.e.,
>> - drop @requires
>> - make line 94-97 to ignore the platform-dependent provider if it’s 
>> present in the white list
>> 
>> If we could make this test order-insensitive, it’d be cleaner to 
>> maintain a platform-neutral list of security providers and one list for 
>> the platform-dependent security providers for each platform.  Just an 
>> idea.
>> 
>> Mandy
>> 
> 

Re: RFR: 8158670: Fix @modules in java/lang/SecurityManager/CheckSecurityProvider.java

[Alexandre (Shura) Iline]

This made sense, than you, Mandy.

Please review new version:
http://cr.openjdk.java.net/~shurailine/8158670/webrev.02/

Shura

> On Jul 2, 2016, at 3:26 PM, Mandy Chung  wrote:
> 
> 
>> On Jul 1, 2016, at 6:20 PM, Alexandre (Shura) Iline 
>>  wrote:
>> 
>> Please review the new version of the fix.
>> http://cr.openjdk.java.net/~shurailine/8158670/webrev.01/
> 
> This looks much better.  Small comment: you can use Layer::findModule and 
> also Optional::ifPresent, like this:
> 
> boot.findModule("jdk.crypto.ucrypto”)
>.ifPresent(m -> 
> expected.add("com.oracle.security.ucrypto.UcryptoProvider”));
> 
> Mandy
> 

Re: RFR: 8158670: Fix @modules in java/lang/SecurityManager/CheckSecurityProvider.java

[Mandy Chung]

> On Jul 1, 2016, at 6:20 PM, Alexandre (Shura) Iline 
>  wrote:
> 
> Please review the new version of the fix.
> http://cr.openjdk.java.net/~shurailine/8158670/webrev.01/

This looks much better.  Small comment: you can use Layer::findModule and also 
Optional::ifPresent, like this:

boot.findModule("jdk.crypto.ucrypto”)
.ifPresent(m -> 
expected.add("com.oracle.security.ucrypto.UcryptoProvider”));

Mandy

Re: RFR: 8158670: Fix @modules in java/lang/SecurityManager/CheckSecurityProvider.java

[Jonathan Gibbons]

The test can also report which providers it is testing and/or skipping, 
so that anyone checking the .jtr file can verify the behavior.


Maybe it fails if expected modules or providers are not found.

-- Jon


On 06/29/2016 10:50 AM, Mandy Chung wrote:

Valerie’s suggestion is a good one.  The test will require a minimum image with 
cross-platform security providers to run the test while it can still verify 
other platform-specific providers.

Mandy


On Jun 29, 2016, at 10:41 AM, Valerie Peng  wrote:


It's not like the test silently passes as the test still covers the 
cross-platform modules.
The way I view this is that the platform=specific modules are "optional" and we 
update the expected result by detecting their presence (or the not). It's not a hack or 
workaround, but rather an enhancement for the test to handle different images.

Just my .02,
Valerie

On 6/29/2016 10:22 AM, Alexandre (Shura) Iline wrote:

On Jun 28, 2016, at 5:22 PM, Valerie Peng  wrote:


One of the purpose of this test is to test the ordering (see the initial bug 
which this test is for: JDK-6997010).

The original test already detects the OS and will skip certain providers 
accordingly.
Instead of splitting the test into multiple platform-specific tests, maybe we 
can keep the original test but add module-presence checking? Is there API 
available to query if certain module are present?

ModuleFinder.ofSystem().find(String).

We can have only the cross-platform modules listed in @modules and make the 
test to pass silently if the required platform-specific modules are not present.

So, for example, on windows, if the test would be executed against an image 
which have no jdk.crypto.mscapi, the test will not run any checks and report 
pass.

This would help to avoid the additional test creation, but it will add another 
silently passing test, which is less clean.

Mandy?

Shura


If yes, then we can leave out the platform-specific providers from the @modules 
line and skip the providers if either the OS does not match or the module is 
not present.

If we can't query what modules are available, then we may have to think of 
something else.
Valerie

On 6/27/2016 12:27 PM, Mandy Chung wrote:

I’m including security-dev which would be a better list to review this test fix.

Valerie,
Does this test have to be order-sensitive?  I think this test would be 
cleaner to make it order-insensitive and simply test the security provider 
initialization.

See my comments below.


On Jun 27, 2016, at 8:21 AM, Alexandre (Shura) Iline 
 wrote:

Hi.

Please take a look on a suggested for for the 
java/lang/SecurityManager/CheckSecurityProvider.java test.

The test in question depend on a list of modules, some of them are 
platform-specific. Listing all the dependencies in one test is causing the test 
to be skipped on every platform. In an offline conversation it was decided that 
it is better to split this tests into a few tests to declare the per-platform 
module dependencies.

The bug: https://bugs.openjdk.java.net/browse/JDK-8158670
The suggested fix: http://cr.openjdk.java.net/~shurailine/8158670/webrev.00/

The copyright header start year of the new tests should be 2016.

I would suggest to make CheckSecurityProvide a platform-neutral test, i.e.,
- drop @requires
- make line 94-97 to ignore the platform-dependent provider if it’s present in 
the white list

If we could make this test order-insensitive, it’d be cleaner to maintain a 
platform-neutral list of security providers and one list for the 
platform-dependent security providers for each platform.  Just an idea.

Mandy

Re: RFR: 8158670: Fix @modules in java/lang/SecurityManager/CheckSecurityProvider.java

[Alexandre (Shura) Iline]

> On Jul 1, 2016, at 6:20 PM, Alexandre (Shura) Iline 
>  wrote:
> 
> Pleas review the new version of the fix.
> http://cr.openjdk.java.net/~shurailine/8158670/webrev.01/
> 
> I have executed the changed test successfully on linux, windows, mac os x and 
> solaris.

I have also executed it manually on my laptop with most relevant values for 
-limitmods. 

Shura

> 
> Shura
> 
>> On Jun 29, 2016, at 10:43 AM, Alexandre (Shura) Iline 
>>  wrote:
>> 
>> 
>>> On Jun 29, 2016, at 10:41 AM, Valerie Peng  wrote:
>>> 
>>> 
>>> It's not like the test silently passes as the test still covers the 
>>> cross-platform modules.
>>> The way I view this is that the platform=specific modules are "optional" 
>>> and we update the expected result by detecting their presence (or the not). 
>>> It's not a hack or workaround, but rather an enhancement for the test to 
>>> handle different images.
>> 
>> Oh … that makes more sense. I mis-understood it originally.
>> 
>> Let me fix it like this.
>> 
>> Shura
>> 
>>> 
>>> Just my .02,
>>> Valerie
>>> 
>>> On 6/29/2016 10:22 AM, Alexandre (Shura) Iline wrote:
> On Jun 28, 2016, at 5:22 PM, Valerie Peng  wrote:
> 
> 
> One of the purpose of this test is to test the ordering (see the initial 
> bug which this test is for: JDK-6997010).
> 
> The original test already detects the OS and will skip certain providers 
> accordingly.
> Instead of splitting the test into multiple platform-specific tests, 
> maybe we can keep the original test but add module-presence checking? Is 
> there API available to query if certain module are present?
 ModuleFinder.ofSystem().find(String).
 
 We can have only the cross-platform modules listed in @modules and make 
 the test to pass silently if the required platform-specific modules are 
 not present.
 
 So, for example, on windows, if the test would be executed against an 
 image which have no jdk.crypto.mscapi, the test will not run any checks 
 and report pass.
 
 This would help to avoid the additional test creation, but it will add 
 another silently passing test, which is less clean.
 
 Mandy?
 
 Shura
 
> If yes, then we can leave out the platform-specific providers from the 
> @modules line and skip the providers if either the OS does not match or 
> the module is not present.
> 
> If we can't query what modules are available, then we may have to think 
> of something else.
> Valerie
> 
> On 6/27/2016 12:27 PM, Mandy Chung wrote:
>> I’m including security-dev which would be a better list to review this 
>> test fix.
>> 
>> Valerie,
>>  Does this test have to be order-sensitive?  I think this test would be 
>> cleaner to make it order-insensitive and simply test the security 
>> provider initialization.
>> 
>> See my comments below.
>> 
>>> On Jun 27, 2016, at 8:21 AM, Alexandre (Shura) Iline 
>>>  wrote:
>>> 
>>> Hi.
>>> 
>>> Please take a look on a suggested for for the 
>>> java/lang/SecurityManager/CheckSecurityProvider.java test.
>>> 
>>> The test in question depend on a list of modules, some of them are 
>>> platform-specific. Listing all the dependencies in one test is causing 
>>> the test to be skipped on every platform. In an offline conversation it 
>>> was decided that it is better to split this tests into a few tests to 
>>> declare the per-platform module dependencies.
>>> 
>>> The bug: https://bugs.openjdk.java.net/browse/JDK-8158670
>>> The suggested fix: 
>>> http://cr.openjdk.java.net/~shurailine/8158670/webrev.00/
>> The copyright header start year of the new tests should be 2016.
>> 
>> I would suggest to make CheckSecurityProvide a platform-neutral test, 
>> i.e.,
>> - drop @requires
>> - make line 94-97 to ignore the platform-dependent provider if it’s 
>> present in the white list
>> 
>> If we could make this test order-insensitive, it’d be cleaner to 
>> maintain a platform-neutral list of security providers and one list for 
>> the platform-dependent security providers for each platform.  Just an 
>> idea.
>> 
>> Mandy
>> 
>>> 
>> 
> 

Re: RFR: 8158670: Fix @modules in java/lang/SecurityManager/CheckSecurityProvider.java

[Alexandre (Shura) Iline]

Pleas review the new version of the fix.
http://cr.openjdk.java.net/~shurailine/8158670/webrev.01/

I have executed the changed test successfully on linux, windows, mac os x and 
solaris.

Shura

> On Jun 29, 2016, at 10:43 AM, Alexandre (Shura) Iline 
>  wrote:
> 
> 
>> On Jun 29, 2016, at 10:41 AM, Valerie Peng  wrote:
>> 
>> 
>> It's not like the test silently passes as the test still covers the 
>> cross-platform modules.
>> The way I view this is that the platform=specific modules are "optional" and 
>> we update the expected result by detecting their presence (or the not). It's 
>> not a hack or workaround, but rather an enhancement for the test to handle 
>> different images.
> 
> Oh … that makes more sense. I mis-understood it originally.
> 
> Let me fix it like this.
> 
> Shura
> 
>> 
>> Just my .02,
>> Valerie
>> 
>> On 6/29/2016 10:22 AM, Alexandre (Shura) Iline wrote:
 On Jun 28, 2016, at 5:22 PM, Valerie Peng  wrote:
 
 
 One of the purpose of this test is to test the ordering (see the initial 
 bug which this test is for: JDK-6997010).
 
 The original test already detects the OS and will skip certain providers 
 accordingly.
 Instead of splitting the test into multiple platform-specific tests, maybe 
 we can keep the original test but add module-presence checking? Is there 
 API available to query if certain module are present?
>>> ModuleFinder.ofSystem().find(String).
>>> 
>>> We can have only the cross-platform modules listed in @modules and make the 
>>> test to pass silently if the required platform-specific modules are not 
>>> present.
>>> 
>>> So, for example, on windows, if the test would be executed against an image 
>>> which have no jdk.crypto.mscapi, the test will not run any checks and 
>>> report pass.
>>> 
>>> This would help to avoid the additional test creation, but it will add 
>>> another silently passing test, which is less clean.
>>> 
>>> Mandy?
>>> 
>>> Shura
>>> 
 If yes, then we can leave out the platform-specific providers from the 
 @modules line and skip the providers if either the OS does not match or 
 the module is not present.
 
 If we can't query what modules are available, then we may have to think of 
 something else.
 Valerie
 
 On 6/27/2016 12:27 PM, Mandy Chung wrote:
> I’m including security-dev which would be a better list to review this 
> test fix.
> 
> Valerie,
>   Does this test have to be order-sensitive?  I think this test would be 
> cleaner to make it order-insensitive and simply test the security 
> provider initialization.
> 
> See my comments below.
> 
>> On Jun 27, 2016, at 8:21 AM, Alexandre (Shura) Iline 
>>  wrote:
>> 
>> Hi.
>> 
>> Please take a look on a suggested for for the 
>> java/lang/SecurityManager/CheckSecurityProvider.java test.
>> 
>> The test in question depend on a list of modules, some of them are 
>> platform-specific. Listing all the dependencies in one test is causing 
>> the test to be skipped on every platform. In an offline conversation it 
>> was decided that it is better to split this tests into a few tests to 
>> declare the per-platform module dependencies.
>> 
>> The bug: https://bugs.openjdk.java.net/browse/JDK-8158670
>> The suggested fix: 
>> http://cr.openjdk.java.net/~shurailine/8158670/webrev.00/
> The copyright header start year of the new tests should be 2016.
> 
> I would suggest to make CheckSecurityProvide a platform-neutral test, 
> i.e.,
> - drop @requires
> - make line 94-97 to ignore the platform-dependent provider if it’s 
> present in the white list
> 
> If we could make this test order-insensitive, it’d be cleaner to maintain 
> a platform-neutral list of security providers and one list for the 
> platform-dependent security providers for each platform.  Just an idea.
> 
> Mandy
> 
>> 
> 

Re: RFR: 8158670: Fix @modules in java/lang/SecurityManager/CheckSecurityProvider.java

[Mandy Chung]

Valerie’s suggestion is a good one.  The test will require a minimum image with 
cross-platform security providers to run the test while it can still verify 
other platform-specific providers.

Mandy

> On Jun 29, 2016, at 10:41 AM, Valerie Peng  wrote:
> 
> 
> It's not like the test silently passes as the test still covers the 
> cross-platform modules.
> The way I view this is that the platform=specific modules are "optional" and 
> we update the expected result by detecting their presence (or the not). It's 
> not a hack or workaround, but rather an enhancement for the test to handle 
> different images.
> 
> Just my .02,
> Valerie
> 
> On 6/29/2016 10:22 AM, Alexandre (Shura) Iline wrote:
>>> On Jun 28, 2016, at 5:22 PM, Valerie Peng  wrote:
>>> 
>>> 
>>> One of the purpose of this test is to test the ordering (see the initial 
>>> bug which this test is for: JDK-6997010).
>>> 
>>> The original test already detects the OS and will skip certain providers 
>>> accordingly.
>>> Instead of splitting the test into multiple platform-specific tests, maybe 
>>> we can keep the original test but add module-presence checking? Is there 
>>> API available to query if certain module are present?
>> ModuleFinder.ofSystem().find(String).
>> 
>> We can have only the cross-platform modules listed in @modules and make the 
>> test to pass silently if the required platform-specific modules are not 
>> present.
>> 
>> So, for example, on windows, if the test would be executed against an image 
>> which have no jdk.crypto.mscapi, the test will not run any checks and report 
>> pass.
>> 
>> This would help to avoid the additional test creation, but it will add 
>> another silently passing test, which is less clean.
>> 
>> Mandy?
>> 
>> Shura
>> 
>>> If yes, then we can leave out the platform-specific providers from the 
>>> @modules line and skip the providers if either the OS does not match or the 
>>> module is not present.
>>> 
>>> If we can't query what modules are available, then we may have to think of 
>>> something else.
>>> Valerie
>>> 
>>> On 6/27/2016 12:27 PM, Mandy Chung wrote:
 I’m including security-dev which would be a better list to review this 
 test fix.
 
 Valerie,
Does this test have to be order-sensitive?  I think this test would be 
 cleaner to make it order-insensitive and simply test the security provider 
 initialization.
 
 See my comments below.
 
> On Jun 27, 2016, at 8:21 AM, Alexandre (Shura) Iline 
>  wrote:
> 
> Hi.
> 
> Please take a look on a suggested for for the 
> java/lang/SecurityManager/CheckSecurityProvider.java test.
> 
> The test in question depend on a list of modules, some of them are 
> platform-specific. Listing all the dependencies in one test is causing 
> the test to be skipped on every platform. In an offline conversation it 
> was decided that it is better to split this tests into a few tests to 
> declare the per-platform module dependencies.
> 
> The bug: https://bugs.openjdk.java.net/browse/JDK-8158670
> The suggested fix: 
> http://cr.openjdk.java.net/~shurailine/8158670/webrev.00/
 The copyright header start year of the new tests should be 2016.
 
 I would suggest to make CheckSecurityProvide a platform-neutral test, i.e.,
 - drop @requires
 - make line 94-97 to ignore the platform-dependent provider if it’s 
 present in the white list
 
 If we could make this test order-insensitive, it’d be cleaner to maintain 
 a platform-neutral list of security providers and one list for the 
 platform-dependent security providers for each platform.  Just an idea.
 
 Mandy
 
> 

Re: RFR: 8158670: Fix @modules in java/lang/SecurityManager/CheckSecurityProvider.java

[Alexandre (Shura) Iline]

> On Jun 29, 2016, at 10:41 AM, Valerie Peng  wrote:
> 
> 
> It's not like the test silently passes as the test still covers the 
> cross-platform modules.
> The way I view this is that the platform=specific modules are "optional" and 
> we update the expected result by detecting their presence (or the not). It's 
> not a hack or workaround, but rather an enhancement for the test to handle 
> different images.

Oh … that makes more sense. I mis-understood it originally.

Let me fix it like this.

Shura

> 
> Just my .02,
> Valerie
> 
> On 6/29/2016 10:22 AM, Alexandre (Shura) Iline wrote:
>>> On Jun 28, 2016, at 5:22 PM, Valerie Peng  wrote:
>>> 
>>> 
>>> One of the purpose of this test is to test the ordering (see the initial 
>>> bug which this test is for: JDK-6997010).
>>> 
>>> The original test already detects the OS and will skip certain providers 
>>> accordingly.
>>> Instead of splitting the test into multiple platform-specific tests, maybe 
>>> we can keep the original test but add module-presence checking? Is there 
>>> API available to query if certain module are present?
>> ModuleFinder.ofSystem().find(String).
>> 
>> We can have only the cross-platform modules listed in @modules and make the 
>> test to pass silently if the required platform-specific modules are not 
>> present.
>> 
>> So, for example, on windows, if the test would be executed against an image 
>> which have no jdk.crypto.mscapi, the test will not run any checks and report 
>> pass.
>> 
>> This would help to avoid the additional test creation, but it will add 
>> another silently passing test, which is less clean.
>> 
>> Mandy?
>> 
>> Shura
>> 
>>> If yes, then we can leave out the platform-specific providers from the 
>>> @modules line and skip the providers if either the OS does not match or the 
>>> module is not present.
>>> 
>>> If we can't query what modules are available, then we may have to think of 
>>> something else.
>>> Valerie
>>> 
>>> On 6/27/2016 12:27 PM, Mandy Chung wrote:
 I’m including security-dev which would be a better list to review this 
 test fix.
 
 Valerie,
Does this test have to be order-sensitive?  I think this test would be 
 cleaner to make it order-insensitive and simply test the security provider 
 initialization.
 
 See my comments below.
 
> On Jun 27, 2016, at 8:21 AM, Alexandre (Shura) Iline 
>  wrote:
> 
> Hi.
> 
> Please take a look on a suggested for for the 
> java/lang/SecurityManager/CheckSecurityProvider.java test.
> 
> The test in question depend on a list of modules, some of them are 
> platform-specific. Listing all the dependencies in one test is causing 
> the test to be skipped on every platform. In an offline conversation it 
> was decided that it is better to split this tests into a few tests to 
> declare the per-platform module dependencies.
> 
> The bug: https://bugs.openjdk.java.net/browse/JDK-8158670
> The suggested fix: 
> http://cr.openjdk.java.net/~shurailine/8158670/webrev.00/
 The copyright header start year of the new tests should be 2016.
 
 I would suggest to make CheckSecurityProvide a platform-neutral test, i.e.,
 - drop @requires
 - make line 94-97 to ignore the platform-dependent provider if it’s 
 present in the white list
 
 If we could make this test order-insensitive, it’d be cleaner to maintain 
 a platform-neutral list of security providers and one list for the 
 platform-dependent security providers for each platform.  Just an idea.
 
 Mandy
 
> 

Re: RFR: 8158670: Fix @modules in java/lang/SecurityManager/CheckSecurityProvider.java

[Valerie Peng]

It's not like the test silently passes as the test still covers the 
cross-platform modules.
The way I view this is that the platform=specific modules are "optional" 
and we update the expected result by detecting their presence (or the 
not). It's not a hack or workaround, but rather an enhancement for the 
test to handle different images.


Just my .02,
Valerie

On 6/29/2016 10:22 AM, Alexandre (Shura) Iline wrote:

On Jun 28, 2016, at 5:22 PM, Valerie Peng  wrote:


One of the purpose of this test is to test the ordering (see the initial bug 
which this test is for: JDK-6997010).

The original test already detects the OS and will skip certain providers 
accordingly.
Instead of splitting the test into multiple platform-specific tests, maybe we 
can keep the original test but add module-presence checking? Is there API 
available to query if certain module are present?

ModuleFinder.ofSystem().find(String).

We can have only the cross-platform modules listed in @modules and make the 
test to pass silently if the required platform-specific modules are not present.

So, for example, on windows, if the test would be executed against an image 
which have no jdk.crypto.mscapi, the test will not run any checks and report 
pass.

This would help to avoid the additional test creation, but it will add another 
silently passing test, which is less clean.

Mandy?

Shura


If yes, then we can leave out the platform-specific providers from the @modules 
line and skip the providers if either the OS does not match or the module is 
not present.

If we can't query what modules are available, then we may have to think of 
something else.
Valerie

On 6/27/2016 12:27 PM, Mandy Chung wrote:

I’m including security-dev which would be a better list to review this test fix.

Valerie,
Does this test have to be order-sensitive?  I think this test would be 
cleaner to make it order-insensitive and simply test the security provider 
initialization.

See my comments below.


On Jun 27, 2016, at 8:21 AM, Alexandre (Shura) Iline 
 wrote:

Hi.

Please take a look on a suggested for for the 
java/lang/SecurityManager/CheckSecurityProvider.java test.

The test in question depend on a list of modules, some of them are 
platform-specific. Listing all the dependencies in one test is causing the test 
to be skipped on every platform. In an offline conversation it was decided that 
it is better to split this tests into a few tests to declare the per-platform 
module dependencies.

The bug: https://bugs.openjdk.java.net/browse/JDK-8158670
The suggested fix: http://cr.openjdk.java.net/~shurailine/8158670/webrev.00/

The copyright header start year of the new tests should be 2016.

I would suggest to make CheckSecurityProvide a platform-neutral test, i.e.,
- drop @requires
- make line 94-97 to ignore the platform-dependent provider if it’s present in 
the white list

If we could make this test order-insensitive, it’d be cleaner to maintain a 
platform-neutral list of security providers and one list for the 
platform-dependent security providers for each platform.  Just an idea.

Mandy

Re: RFR: 8158670: Fix @modules in java/lang/SecurityManager/CheckSecurityProvider.java

[Valerie Peng]

One of the purpose of this test is to test the ordering (see the initial 
bug which this test is for: JDK-6997010).


The original test already detects the OS and will skip certain providers 
accordingly.
Instead of splitting the test into multiple platform-specific tests, 
maybe we can keep the original test but add module-presence checking? Is 
there API available to query if certain module are present?
If yes, then we can leave out the platform-specific providers from the 
@modules line and skip the providers if either the OS does not match or 
the module is not present.


If we can't query what modules are available, then we may have to think 
of something else.

Valerie

On 6/27/2016 12:27 PM, Mandy Chung wrote:

I’m including security-dev which would be a better list to review this test fix.

Valerie,
Does this test have to be order-sensitive?  I think this test would be 
cleaner to make it order-insensitive and simply test the security provider 
initialization.

See my comments below.


On Jun 27, 2016, at 8:21 AM, Alexandre (Shura) Iline 
 wrote:

Hi.

Please take a look on a suggested for for the 
java/lang/SecurityManager/CheckSecurityProvider.java test.

The test in question depend on a list of modules, some of them are 
platform-specific. Listing all the dependencies in one test is causing the test 
to be skipped on every platform. In an offline conversation it was decided that 
it is better to split this tests into a few tests to declare the per-platform 
module dependencies.

The bug: https://bugs.openjdk.java.net/browse/JDK-8158670
The suggested fix: http://cr.openjdk.java.net/~shurailine/8158670/webrev.00/

The copyright header start year of the new tests should be 2016.

I would suggest to make CheckSecurityProvide a platform-neutral test, i.e.,
- drop @requires
- make line 94-97 to ignore the platform-dependent provider if it’s present in 
the white list

If we could make this test order-insensitive, it’d be cleaner to maintain a 
platform-neutral list of security providers and one list for the 
platform-dependent security providers for each platform.  Just an idea.

Mandy

Re: RFR: 8158670: Fix @modules in java/lang/SecurityManager/CheckSecurityProvider.java

[Alexandre (Shura) Iline]

Copyrights fixed in place.

Thank you, Mandy.

Shura

> On Jun 27, 2016, at 12:27 PM, Mandy Chung  wrote:
> 
> I’m including security-dev which would be a better list to review this test 
> fix.
> 
> Valerie,
>   Does this test have to be order-sensitive?  I think this test would be 
> cleaner to make it order-insensitive and simply test the security provider 
> initialization.
> 
> See my comments below.
> 
>> On Jun 27, 2016, at 8:21 AM, Alexandre (Shura) Iline 
>>  wrote:
>> 
>> Hi.
>> 
>> Please take a look on a suggested for for the 
>> java/lang/SecurityManager/CheckSecurityProvider.java test. 
>> 
>> The test in question depend on a list of modules, some of them are 
>> platform-specific. Listing all the dependencies in one test is causing the 
>> test to be skipped on every platform. In an offline conversation it was 
>> decided that it is better to split this tests into a few tests to declare 
>> the per-platform module dependencies.
>> 
>> The bug: https://bugs.openjdk.java.net/browse/JDK-8158670
>> The suggested fix: http://cr.openjdk.java.net/~shurailine/8158670/webrev.00/
> 
> The copyright header start year of the new tests should be 2016.
> 
> I would suggest to make CheckSecurityProvide a platform-neutral test, i.e.,
> - drop @requires
> - make line 94-97 to ignore the platform-dependent provider if it’s present 
> in the white list
> 
> If we could make this test order-insensitive, it’d be cleaner to maintain a 
> platform-neutral list of security providers and one list for the 
> platform-dependent security providers for each platform.  Just an idea.
> 
> Mandy
> 

Re: RFR: 8158670: Fix @modules in java/lang/SecurityManager/CheckSecurityProvider.java

[Mandy Chung]

I’m including security-dev which would be a better list to review this test fix.

Valerie,
   Does this test have to be order-sensitive?  I think this test would be 
cleaner to make it order-insensitive and simply test the security provider 
initialization.

See my comments below.

> On Jun 27, 2016, at 8:21 AM, Alexandre (Shura) Iline 
>  wrote:
> 
> Hi.
> 
> Please take a look on a suggested for for the 
> java/lang/SecurityManager/CheckSecurityProvider.java test. 
> 
> The test in question depend on a list of modules, some of them are 
> platform-specific. Listing all the dependencies in one test is causing the 
> test to be skipped on every platform. In an offline conversation it was 
> decided that it is better to split this tests into a few tests to declare the 
> per-platform module dependencies.
> 
> The bug: https://bugs.openjdk.java.net/browse/JDK-8158670
> The suggested fix: http://cr.openjdk.java.net/~shurailine/8158670/webrev.00/

The copyright header start year of the new tests should be 2016.

I would suggest to make CheckSecurityProvide a platform-neutral test, i.e.,
- drop @requires
- make line 94-97 to ignore the platform-dependent provider if it’s present in 
the white list

If we could make this test order-insensitive, it’d be cleaner to maintain a 
platform-neutral list of security providers and one list for the 
platform-dependent security providers for each platform.  Just an idea.

Mandy