RFR 8178323: Add negative tests for bind services Jlink feature

[Andrey Nazarov]

Hi, 

Please review 3 negative tests for Jlink which tests new 
bind-services/suggest-providers feature.

JBS: https://bugs.openjdk.java.net/browse/JDK-8178323

webrev: http://cr.openjdk.java.net/~anazarov/8178323/webrev.00/webrev/ 


—Andrey

Re: RFR 8178323: Add negative tests for bind services Jlink feature

[Mandy Chung]

> On Apr 7, 2017, at 8:40 AM, Andrey Nazarov  
> wrote:
> 
> Hi, 
> 
> Please review 3 negative tests for Jlink which tests new 
> bind-services/suggest-providers feature.
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8178323
> 
> webrev: http://cr.openjdk.java.net/~anazarov/8178323/webrev.00/webrev/ 

Thanks for adding these tests.

BindServices.java

 155 Path dir = Paths.get("verboseNoop”);

I suggest to rename “verboseNoop” to “verboseNoBind”

line 159-161: formatting nit: can you add spaces to align with the first
argument in line 158.  Same comment to SuggestProviders.java line 180-182
an 198-200.

SuggestProviders.java
  It may be better to rename “suggestNotProvider" to “noSuggestedProvider".

  In the noOneUsesProvider test case, is m4 not observable? I think 
jlink should fail with m4 not found. When —-suggest-providers is 
specified with a service type, it’s a bug in the implementation that 
does not report it.  This should be renamed to “nonObservableModule”
instead.  We should file a bug and include this test case in the JBS report.

Mandy

Re: RFR 8178323: Add negative tests for bind services Jlink feature

[Andrey Nazarov]

Mandy, thanks for review. I’ve updated patch
http://cr.openjdk.java.net/~anazarov/8178323/webrev.01/webrev/ 


See my comments inline
> On 8 Apr 2017, at 03:18, Mandy Chung  wrote:
> 
> 
>> On Apr 7, 2017, at 8:40 AM, Andrey Nazarov  
>> wrote:
>> 
>> Hi, 
>> 
>> Please review 3 negative tests for Jlink which tests new 
>> bind-services/suggest-providers feature.
>> 
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8178323
>> 
>> webrev: http://cr.openjdk.java.net/~anazarov/8178323/webrev.00/webrev/ 
> 
> Thanks for adding these tests.
> 
> BindServices.java
> 
> 155 Path dir = Paths.get("verboseNoop”);
> 
> I suggest to rename “verboseNoop” to “verboseNoBind”
fixed
> 
> line 159-161: formatting nit: can you add spaces to align with the first
> argument in line 158.  Same comment to SuggestProviders.java line 180-182
> an 198-200.
Fixed
> 
> SuggestProviders.java
>  It may be better to rename “suggestNotProvider" to “noSuggestedProvider”.
We already have test with name noSuggestProvider. Idea here was to specify real 
existing type which is not provider according to module-info.java. I’ve renamed 
to “suggestTypeNotRealProvider”.
> 
>  In the noOneUsesProvider test case, is m4 not observable? I think 
> jlink should fail with m4 not found. When —-suggest-providers is 
> specified with a service type, it’s a bug in the implementation that 
> does not report it.  This should be renamed to “nonObservableModule”
> instead.  We should file a bug and include this test case in the JBS report.
Actually there are two issues here: 
https://bugs.openjdk.java.net/browse/JDK-8178404 

https://bugs.openjdk.java.net/browse/JDK-8178405

> 
> Mandy

Re: RFR 8178323: Add negative tests for bind services Jlink feature

[Mandy Chung]

> On Apr 10, 2017, at 12:30 PM, Andrey Nazarov  
> wrote:
> 
> Mandy, thanks for review. I’ve updated patch
> http://cr.openjdk.java.net/~anazarov/8178323/webrev.01/webrev/ 
> 
> 

I suggest to add m4/p4.S to make it clear it’s a service type and m4 provides 
p4.S with  p4.Impl.
noOneUsesProvider will call —suggest-providers p4.S instead.

 239 static JLink run(boolean expectSuccess, String... options) {
Instead of adding a new parameter, what about adding a new 
runWithNonZeroExitCode method?

> See my comments inline
>> 
>> 
>> line 159-161: formatting nit: can you add spaces to align with the first
>> argument in line 158.  Same comment to SuggestProviders.java line 180-182
>> an 198-200.
> Fixed

line 216-219.

> Actually there are two issues here: 
> https://bugs.openjdk.java.net/browse/JDK-8178404 
> 
> https://bugs.openjdk.java.net/browse/JDK-8178405 
> 
Not really a bug but it’d be helpful to show that the provider exists but the 
service is not used in this case.
Mandy

Re: RFR 8178323: Add negative tests for bind services Jlink feature

[Andrey Nazarov]

Hi Mandy,

Here is updated patch 
http://cr.openjdk.java.net/~anazarov/8178323/webrev.02/webrev/ 


— Thanks,
Andrey
> On 10 Apr 2017, at 23:50, Mandy Chung  > wrote:
> 
> 
>> On Apr 10, 2017, at 12:30 PM, Andrey Nazarov > > wrote:
>> 
>> Mandy, thanks for review. I’ve updated patch
>> http://cr.openjdk.java.net/~anazarov/8178323/webrev.01/webrev/ 
>> 
>> 
> 
> I suggest to add m4/p4.S to make it clear it’s a service type and m4 provides 
> p4.S with  p4.Impl.
> noOneUsesProvider will call —suggest-providers p4.S instead.
> 
>  239 static JLink run(boolean expectSuccess, String... options) {
> Instead of adding a new parameter, what about adding a new 
> runWithNonZeroExitCode method?
> 
>> See my comments inline
>>> 
>>> 
>>> line 159-161: formatting nit: can you add spaces to align with the first
>>> argument in line 158.  Same comment to SuggestProviders.java line 180-182
>>> an 198-200.
>> Fixed
> 
> line 216-219.
> 
>> Actually there are two issues here: 
>> https://bugs.openjdk.java.net/browse/JDK-8178404 
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8178405 
>> 
> Not really a bug but it’d be helpful to show that the provider exists but the 
> service is not used in this case.
> Mandy
> 

Re: RFR 8178323: Add negative tests for bind services Jlink feature

[Andrey Nazarov]

Hi Mandy,

Here is updated patch 
http://cr.openjdk.java.net/~anazarov/8178323/webrev.02/webrev/ 


— Thanks,
Andrey
> On 10 Apr 2017, at 23:50, Mandy Chung  wrote:
> 
> 
>> On Apr 10, 2017, at 12:30 PM, Andrey Nazarov > > wrote:
>> 
>> Mandy, thanks for review. I’ve updated patch
>> http://cr.openjdk.java.net/~anazarov/8178323/webrev.01/webrev/ 
>> 
>> 
> 
> I suggest to add m4/p4.S to make it clear it’s a service type and m4 provides 
> p4.S with  p4.Impl.
> noOneUsesProvider will call —suggest-providers p4.S instead.
> 
>  239 static JLink run(boolean expectSuccess, String... options) {
> Instead of adding a new parameter, what about adding a new 
> runWithNonZeroExitCode method?
> 
>> See my comments inline
>>> 
>>> 
>>> line 159-161: formatting nit: can you add spaces to align with the first
>>> argument in line 158.  Same comment to SuggestProviders.java line 180-182
>>> an 198-200.
>> Fixed
> 
> line 216-219.
> 
>> Actually there are two issues here: 
>> https://bugs.openjdk.java.net/browse/JDK-8178404 
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8178405 
>> 
> Not really a bug but it’d be helpful to show that the provider exists but the 
> service is not used in this case.
> Mandy
> 

Re: RFR 8178323: Add negative tests for bind services Jlink feature

[Andrey Nazarov]

HI Mandy,

I’ve update patch 
http://cr.openjdk.java.net/~anazarov/8178323/webrev.03/webrev/ 

I’ve removed some tests since you pushed them with fix for 
https://bugs.openjdk.java.net/browse/JDK-8178404

—Andrey
> On 11 Apr 2017, at 12:46, Andrey Nazarov  wrote:
> 
> Hi Mandy,
> 
> Here is updated patch 
> http://cr.openjdk.java.net/~anazarov/8178323/webrev.02/webrev/ 
> 
> 
> — Thanks,
> Andrey
>> On 10 Apr 2017, at 23:50, Mandy Chung  wrote:
>> 
>> 
>>> On Apr 10, 2017, at 12:30 PM, Andrey Nazarov >> > wrote:
>>> 
>>> Mandy, thanks for review. I’ve updated patch
>>> http://cr.openjdk.java.net/~anazarov/8178323/webrev.01/webrev/ 
>>> 
>>> 
>> 
>> I suggest to add m4/p4.S to make it clear it’s a service type and m4 
>> provides p4.S with  p4.Impl.
>> noOneUsesProvider will call —suggest-providers p4.S instead.
>> 
>> 239 static JLink run(boolean expectSuccess, String... options) {
>> Instead of adding a new parameter, what about adding a new 
>> runWithNonZeroExitCode method?
>> 
>>> See my comments inline
 
 
 line 159-161: formatting nit: can you add spaces to align with the first
 argument in line 158.  Same comment to SuggestProviders.java line 180-182
 an 198-200.
>>> Fixed
>> 
>> line 216-219.
>> 
>>> Actually there are two issues here: 
>>> https://bugs.openjdk.java.net/browse/JDK-8178404 
>>> 
>>> https://bugs.openjdk.java.net/browse/JDK-8178405 
>>> 
>> Not really a bug but it’d be helpful to show that the provider exists but 
>> the service is not used in this case.
>> Mandy
>> 
> 

Re: RFR 8178323: Add negative tests for bind services Jlink feature

[Mandy Chung]

> On Apr 20, 2017, at 5:12 PM, Andrey Nazarov  
> wrote:
> 
> HI Mandy,
> 
> I’ve update patch 
> http://cr.openjdk.java.net/~anazarov/8178323/webrev.03/webrev/ 
> 
Nit: use 4-space indent e.g. BindServices.java. line 159 (the following lines 
will move along) and SuggestProviders.java line 255-258, 262, 273-276,  and 280.

Otherwise looks good.

You can fix the formatting before push.

Thanks
Mandy