Quantcast

RFR 8087307/9, new tests for ServiceLoader updates for module

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR 8087307/9, new tests for ServiceLoader updates for module

FELIX YANG
Hi there,

     please review the new tests added for ServiceLoader updates for
module system.

Test bug:

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

Webrev:

     http://cr.openjdk.java.net/~xiaofeya/8087307/webrev.00/

Related product Changes:

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

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

Thanks,

Felix


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8087307/9, new tests for ServiceLoader updates for module

FELIX YANG
Ping:)

-Felix

> On 16 May 2017, at 4:59 PM, Felix Yang <[hidden email]> wrote:
>
> Hi there,
>
>    please review the new tests added for ServiceLoader updates for module system.
>
> Test bug:
>
>    https://bugs.openjdk.java.net/browse/JDK-8087307
>
> Webrev:
>
>    http://cr.openjdk.java.net/~xiaofeya/8087307/webrev.00/
>
> Related product Changes:
>
>    https://bugs.openjdk.java.net/browse/JDK-8132026
>
>    https://bugs.openjdk.java.net/browse/JDK-8047814
>
> Thanks,
>
> Felix
>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8087307/9, new tests for ServiceLoader updates for module

Amy Lu-2
Not a reviewer.

Just noticed that tests don't have @modules.
Is it better to include @modules when introducing new test?

Thanks,
Amy

On 5/19/17 9:38 AM, Felix Yang wrote:

> Ping:)
>
> -Felix
>> On 16 May 2017, at 4:59 PM, Felix Yang <[hidden email]> wrote:
>>
>> Hi there,
>>
>>     please review the new tests added for ServiceLoader updates for module system.
>>
>> Test bug:
>>
>>     https://bugs.openjdk.java.net/browse/JDK-8087307
>>
>> Webrev:
>>
>>     http://cr.openjdk.java.net/~xiaofeya/8087307/webrev.00/
>>
>> Related product Changes:
>>
>>     https://bugs.openjdk.java.net/browse/JDK-8132026
>>
>>     https://bugs.openjdk.java.net/browse/JDK-8047814
>>
>> Thanks,
>>
>> Felix
>>
>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8087307/9, new tests for ServiceLoader updates for module

Hamlin Li
In reply to this post by FELIX YANG
Hi Felix,

I have some comments as below:


1. Possible test gaps for Locating Order
         1.1 providers in named module have high priority than providers
in unnamed
         1.2 ServiceLoader.load​(ModuleLayer layer, Class<S> service) is
not tested
2. Possible test gap for automatic module, it is not tested

3. Could you remove unnecessary module dependency on java.scripting,
replace it with a customized module or java.base? it will make your
tests run under as many as conditions.

4. For [a|b]filesystem
     could you use URLStreamHandlerProvider rather than
FileSystemProvider, it will make your test files(FileSystemForInstr,
AFileSystem, BFileSystem) more simple.

5. MisUsesTest.java and missuse
     missuses: miss => mis? (missuses => mis.uses)
     Possible test gap: add similar use case for unnamed module?

6. OrderingWithInstrTest.java
     Could you use jtreg tag "driver RedefineModuleAgent" rather than
"@run main/othervm OrderingWithInstrTest" to trigger the preparation of
test? and move createAgentJar() and main() into RedefineModuleAgent.java.

7. ReloadTest.java
     The test success depends on execution order of tests, so need to
add priority=N to every @Test method, or separate tests into 2 classes,
one for negative, one for normal tests.

8. Some additional minor comments:
     8.1 split module name with ".", for e.g. multiprovides =>
multi.provides
     8.2 wrap long lines, and revert unnecessary wrapping too, in
several files.
     8.3 correct indent, e.g. in PermissionTest.java
     8.4 Is default constructor "public ProviderX() { }" necessary in
multi.ProviderX ?
     8.5 rename Service to OrderedService?

Thank you

-Hamlin


On 2017/5/19 9:38, Felix Yang wrote:

> Ping:)
>
> -Felix
>> On 16 May 2017, at 4:59 PM, Felix Yang <[hidden email]> wrote:
>>
>> Hi there,
>>
>>     please review the new tests added for ServiceLoader updates for module system.
>>
>> Test bug:
>>
>>     https://bugs.openjdk.java.net/browse/JDK-8087307
>>
>> Webrev:
>>
>>     http://cr.openjdk.java.net/~xiaofeya/8087307/webrev.00/
>>
>> Related product Changes:
>>
>>     https://bugs.openjdk.java.net/browse/JDK-8132026
>>
>>     https://bugs.openjdk.java.net/browse/JDK-8047814
>>
>> Thanks,
>>
>> Felix
>>
>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8087307/9, new tests for ServiceLoader updates for module

Alan Bateman
In reply to this post by FELIX YANG
On 19/05/2017 02:38, Felix Yang wrote:

> Ping:)
>
> -Felix
>
I have this on my list to see how it fits with the other tests that we
did for SL in JDK 9. From a quick scan then some clean-up will be needed
and I think we will need to change some of the service types to allow
these tests be easily maintained.

-Alan
Loading...