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.y...@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
