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

[Felix Yang]

Hi all,

     updated tests according with comments. Also comments inline.

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

Thanks,

Felix

On 2017/5/19 13:57, Hamlin Li wrote:
> 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
This is a set of additional tests.  The above scenarios have been 
covered in existing j.u.ServiceLoader/modules/Basic.java

-Felix
> 2. Possible test gap for automatic module, it is not tested
>
Add a basic test for providers in automatic module. It leverages 
existing scripts implementations in other tests.

> 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.
Good catch,thanks
-Felix
>
> 4. For [a|b]filesystem
>     could you use URLStreamHandlerProvider rather than 
> FileSystemProvider, it will make your test files(FileSystemForInstr, 
> AFileSystem, BFileSystem) more simple.
This is intended to leverage JDK built-in providers (jar, jrt) in 
Platform and Bootstrap classloaders. There is no explicit implementation 
for URLStreamHandlerProvider.
-Felix
>
> 5. MisUsesTest.java and missuse
>     missuses: miss => mis? (missuses => mis.uses)
>     Possible test gap: add similar use case for unnamed module?
A good question, but I think it is not necessary. The test is to cover 
missing "uses" in module descriptor with a service 
(URLStreamHandlerProvider) having no explicit implementation. It 
implicitly indicates that the "can use" check not-related with how it is 
implemented.

-Felix
>
> 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.
Fixed, thanks.
-Felix
>
> 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.
The tests looks for me not depend on order. They only depend on setup 
method which is annotated with BeforeClass.
>
> 8. Some additional minor comments:
>     8.1 split module name with ".", for e.g. multiprovides => 
> multi.provides
I may keep the naming to be identical with existing ServiceLoader module 
tests.
>     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 ?
Resolved above, thanks

> 8.5 rename Service to OrderedService?
I may keep to be identical with existing ServiceLoader module tests.
-Felix
>
> Thank you
>
> -Hamlin
>
>
> On 2017/5/19 9:38, Felix Yang wrote:
>> Ping:)
>>
>> -Felix
>>> On 16 May 2017, at 4:59 PM, Felix Yang <felix.yang at oracle.com> 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
>>>
>>>
>