Re: Review Request: 8179025: Exclude deployment modules from FieldSetAccessibleTest.java and VerifyJimage.java

[Alan Bateman]

On 25/04/2017 02:58, Mandy Chung wrote:


:
Indeed looks a little odd.  I prefer using isPresent since this method intends 
to return true if the target is not in the resolved graph.

return !cf.findModule(target).isPresent();


Lots of ways to this, what you have looks okay to me.

-Alan

Re: Review Request: 8179025: Exclude deployment modules from FieldSetAccessibleTest.java and VerifyJimage.java

[Mandy Chung]

> On Apr 23, 2017, at 4:09 AM, Alan Bateman  wrote:
> 
> On 22/04/2017 16:42, Mandy Chung wrote:
> 
>> Have an explicit list is another alternative.  OTOH I think only deployment 
>> modules will name with these words though which was what I initially want to 
>> cover.
>> 
>> Since only 4 modules we are concerned about,  I updated the patch to list 
>> the ones needed to be excluded rather than a superset as you suggest:
>> 
>> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8179025/webrev.01/
>> 
>> 
> The updated filter looks okay to me.
> 
> In JdkQualifiedExportTest.accept then "cf.findModule(target).orElse(null) == 
> null" looks a bit odd (I assume this is what promoted Peter bringing up 
> isEmpty on core-libs-dev.  There are a dozen ways you could do this of 
> course, only alternative is:
>return cf.findModule(target).map(m -> false).orElse(true);

Indeed looks a little odd.  I prefer using isPresent since this method intends 
to return true if the target is not in the resolved graph.

   return !cf.findModule(target).isPresent();

Mandy

Re: Review Request: 8179025: Exclude deployment modules from FieldSetAccessibleTest.java and VerifyJimage.java

[Alan Bateman]

On 22/04/2017 16:42, Mandy Chung wrote:


Have an explicit list is another alternative.  OTOH I think only deployment 
modules will name with these words though which was what I initially want to 
cover.

Since only 4 modules we are concerned about,  I updated the patch to list the 
ones needed to be excluded rather than a superset as you suggest:

http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8179025/webrev.01/



The updated filter looks okay to me.

In JdkQualifiedExportTest.accept then 
"cf.findModule(target).orElse(null) == null" looks a bit odd (I assume 
this is what promoted Peter bringing up isEmpty on core-libs-dev.  There 
are a dozen ways you could do this of course, only alternative is:

return cf.findModule(target).map(m -> false).orElse(true);

-Alan

Re: Review Request: 8179025: Exclude deployment modules from FieldSetAccessibleTest.java and VerifyJimage.java

[Mandy Chung]

Have an explicit list is another alternative.  OTOH I think only deployment 
modules will name with these words though which was what I initially want to 
cover.

Since only 4 modules we are concerned about,  I updated the patch to list the 
ones needed to be excluded rather than a superset as you suggest:

http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8179025/webrev.01/

thanks
Mandy

> On Apr 22, 2017, at 1:22 AM, Peter Levart  wrote:
> 
> Hi Mandy,
> 
> In order to make the FieldSetAccessibleTest more resilient to future changes 
> (i.e. adding / renaming modules), perhaps the modules to be excluded in the 
> check should be explicitly listed by their names? Currently your rule, when 
> negated, lists the following modules:
> 
> javafx.deploy
> jdk.deploy
> jdk.javaws
> jdk.plugin.dom
> jdk.plugin
> jdk.deploy.controlpanel
> jdk.plugin.server
> 
> ...which is not to much to put in a Set.of() instance.
> 
> There's no harm if future changes forget to add/change this set, but it would 
> be wrong if the rule you have now, inadvertently excludes some future module 
> that should be checked.
> 
> Regards, Peter
> 
> 
> On 04/21/2017 10:53 PM, Mandy Chung wrote:
>> Webrev:
>>
>> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8179025/webrev.00/index.html 
>> 
>> 
>> These tests failed due to IAE when loading types from the deployment 
>> modules which are expected to be defined when running with javaws
>> or plugin.  This revises the tests to exclude these modules to
>> remove the tests from the problem list.  In the long term, we 
>> should look into some way not to link in these modules in the image.
>> 
>> This patch also updates JdkQualifiedExportTest.java test to take out
>> the exception for deployment modules to have qualified exports to 
>> upgradeable modules.
>> 
>> thanks
>> Mandy
> 

Re: Review Request: 8179025: Exclude deployment modules from FieldSetAccessibleTest.java and VerifyJimage.java

[Peter Levart]

Hi Mandy,

In order to make the FieldSetAccessibleTest more resilient to future 
changes (i.e. adding / renaming modules), perhaps the modules to be 
excluded in the check should be explicitly listed by their names? 
Currently your rule, when negated, lists the following modules:


javafx.deploy
jdk.deploy
jdk.javaws
jdk.plugin.dom
jdk.plugin
jdk.deploy.controlpanel
jdk.plugin.server

...which is not to much to put in a Set.of() instance.

There's no harm if future changes forget to add/change this set, but it 
would be wrong if the rule you have now, inadvertently excludes some 
future module that should be checked.


Regards, Peter


On 04/21/2017 10:53 PM, Mandy Chung wrote:

Webrev:
http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8179025/webrev.00/index.html

These tests failed due to IAE when loading types from the deployment
modules which are expected to be defined when running with javaws
or plugin.  This revises the tests to exclude these modules to
remove the tests from the problem list.  In the long term, we
should look into some way not to link in these modules in the image.

This patch also updates JdkQualifiedExportTest.java test to take out
the exception for deployment modules to have qualified exports to
upgradeable modules.

thanks
Mandy

Review Request: 8179025: Exclude deployment modules from FieldSetAccessibleTest.java and VerifyJimage.java

[Mandy Chung]

Webrev:
   http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8179025/webrev.00/index.html

These tests failed due to IAE when loading types from the deployment 
modules which are expected to be defined when running with javaws
or plugin.  This revises the tests to exclude these modules to
remove the tests from the problem list.  In the long term, we 
should look into some way not to link in these modules in the image.

This patch also updates JdkQualifiedExportTest.java test to take out
the exception for deployment modules to have qualified exports to 
upgradeable modules.

thanks
Mandy