Hi Stuart,
thanks a lot for the comments!-Felix On 2016/4/28 9:06, Stuart Marks wrote:
It was chosen to compile once considering efficiency. If we prefer each test can be executed separately, it looks enough to just change "@BeforeTest" to "@BeforeMethod" and update tests slightly. With jar files, may I understand it is a different scenario with automatic modules. I suggest to add a new test in future enhancement.Hi Felix,I finally got a chance to take a good look at this. Again, sorry for the delays. I was able to reproduce the failure and find a fix for it. There are also some structural issues with the test that will require some improvements.* * *First is the way the test fixture is set up. The test now has a single copy of the source code for the client, server, and app, and they're compiled once. This is good. Unfortunately, the tests modify the filesystem in order to set up different cases for the classes' modular structure.The problem is that makes the tests dependent on each other. The Test-NG directives enforce this execution order, but it makes them really hard to work with. For example, if I were to disable test #2, this would cause test #3 to fail since #3 depends on side effects from test #2. Also, if test #2 were to fail, it's hard to debug, since test #3 modifies the fixture after test #2 runs, potentially obscuring any problems that #2 might have run into.Ideally the test should set up a single test fixture and not modify it, allowing the different cases to be run independently. Offhand I'm not sure of the best way to do this.One approach that might be worth exploring is to create several different jar files from the class files; the jar files would have the classes in different modular configurations. Then, each test would invoke a JVM with arguments to put the different jar files on the classpath or the module path. I haven't investigated this approach, though, so I don't know if it's practical.Other approaches may be viable as well. It's probably worth discussing and prototyping an approach first before you spend too much time implementing any particular scheme.
See http://cr.openjdk.java.net/~xiaofeya/8078812/webrev.02 -Felix
Thanks for figuring this out. But do you think it is worth to investigate the " NoClassDefFoundError " and "A JNI error has occurred". The root cause is not well-indicated.* * *Moving on to the actual test failure, the message from testUnnamedApp() is as follows:==================================================Command line: [/Users/src/jdk-dev/jdk9-dev/build/macosx-x86_64-normal-server-fastdebug/jdk/bin/java -ea -esa -mp mods -Djava.rmi.server.codebase=file:/Users/src/jdk-dev/jdk9-dev/jdk/testoutput/JTwork/java/rmi/module/ModuleTest/./mods/mserver/ -cp classes testpkg.DummyApp 65071 ]Error: A JNI error has occurred, please check your installation and try again Exception in thread "main" java.lang.NoClassDefFoundError: serverpkg/Helloat java.lang.Class.getDeclaredMethods0(java.base/Native Method)at java.lang.Class.privateGetDeclaredMethods(java.base/Class.java:2937) at java.lang.Class.privateGetMethodRecursive(java.base/Class.java:3282)at java.lang.Class.getMethod0(java.base/Class.java:3252) at java.lang.Class.getMethod(java.base/Class.java:1961)at sun.launcher.LauncherHelper.validateMainClass(java.base/LauncherHelper.java:648) at sun.launcher.LauncherHelper.checkAndLoadMain(java.base/LauncherHelper.java:499)Caused by: java.lang.ClassNotFoundException: serverpkg.Helloat jdk.internal.loader.BuiltinClassLoader.loadClass(java.base/BuiltinClassLoader.java:366) at jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(java.base/ClassLoaders.java:184)at java.lang.ClassLoader.loadClass(java.base/ClassLoader.java:419) ... 7 more test ModuleTest.testUnnamedApp(): failure java.lang.AssertionError: expected [0] but found [1] [remainder of stack trace elided] ==================================================The main class is testpkg.DummyApp, but the launcher fails with NoClassDefFoundError on serverpkg/Hello. This was a bit of a puzzle, but I finally figured out that this is occurring during the verification of the DummyApp class. DummyApp depends on Hello, which can't be found, so it fails verification before DummyApp.main() gets called.It's also strange that it says "A JNI error has occurred" but that might be a red herring.In any case the real issue is that serverpkg.Hello can't be found. The reason it can't be found is that it's in a module. That module is on the module path, but the module isn't resolved by default. Thus its classes aren't visible to classes in the unnamed module.To add modules to the set of resolved modules, use the -addmods option. In this case, testUnnamedApp() has the client and server classes in modules, so it needs to have the arguments-addmods mclient,mserveradded to its command line. Since the mclient module explicitly depends on the mserver module, this could instead just e-addmods mclientThe testUnnamedAppandClient() method has only the server in a named module, so it needs to have-addmods mserver on its command line. With these changes, all the tests pass.
-Felix
Excuse me, could you explain what to remove in detail? I agree the scenarios can be various beyond the sanity ones.* * *There are a couple other things that could be improved as well. I'll just mention them here (so I don't forget) but we should work on these later:1) The registry and codebase stuff is probably unnecessary and can be removed. These might be interesting test cases for the future. It's not clear to me that we need to support modules in the RMI codebase, but at least there should be tests for cases we think are important.
-Felix
There is explicit System.exit(-1) for error situation, so I didn't observe such stale process. Any way, I updated it slightly.2) In DummyApp, if creating the Client fails (e.g., because of NoClassDefFoundError), that error will propagate out of the main() method, bypassing the calls to System.exit(). The Server object has already been exported, which will cause the JVM to live indefinitely. This causes the test to hang and eventually time out, and to leave stale JVM processes after the test run.
-Felix
In any case, I think the next step is to come up with a good design for varying the modular structure of the classes for the different test cases, without modifying the filesystem. Let's work on that, and we can work on the other stuff later.s'marks On 4/14/16 1:45 AM, Felix Yang wrote:Hi Stuart, any comment on the new tests and issue? Thanks, Felix On 2016/4/8 15:52, Felix Yang wrote:Hi Stuart,thanks for the comments. I adjusted the test. The test failed with NoClassDefFoundErr if client/server are in named module but dummy app inunnamed module. See https://bugs.openjdk.java.net/browse/JDK-8153833 New webrev: http://cr.openjdk.java.net/~xiaofeya/8078812/webrev.01/ <http://cr.openjdk.java.net/%7Exiaofeya/8078812/webrev.01/>About simplifying the scenario, I replaced inheritance by moving classes inruntime. Thanks, Felix On 2016/3/2 10:28, Stuart Marks wrote:Hi Felix, sorry for the delay. Several comments below. -------------------------------------------------- 119 // wait for rmiregistry to be fully ready 120 Thread.sleep(3000);There are some RMI test utilities that will handle starting up and waiting for the registry to be ready. Unfortunately they're in the unnamed package(still haven't cleaned that up) so you can't use them from this test.For now I'd recommend scaling the timeout by the test.timeout.factor, so thatthis won't fail on slow systems. There's a test utility for that; see Utils.adjustTimeout(). --------------------------------------------------The directory "unamed" is misspelled; it has classes in the unnamed module.This misspelling is reflected in variable names and values, e.g.,59 private static final Path UNAMED_SRC_DIR = Paths.get(TEST_SRC,"unamed");60 private static final Path MODS_OUTPUT_DIR = Paths.get("mods"); 61 private static final Path UNAMED_OUTPUT_DIR = Paths.get("unamed");While spelling errors aren't that big a deal, since this involves file and directory names, I'd recommend fixing this now. It'll just be harder to fixit later. Also, "SeperateModuleClient" => "SeparateModuleClient" --------------------------------------------------Overall the structure of the test seems reasonable to test various clients and servers in combinations of the same, different, and unnamed modules.I'm not entirely sure what p2.SeperateModuleClient is testing. It extendsp1.Client and its constructor and testStub() method simply call thecorresponding methods on super, so it doesn't seem to be testing anythingdifferent from p1.Client running against p3.Server.Also, p4.UnamedModuleClient seems to want to run the client from the unnamed module, but it too extends p1.Client and simply defers all of its execution to its superclass. So I don't think this actually testing an RMI client calloriginating from an unnamed module. There is an uncomfortable amount of duplication betweenmtest/test/DummyApp.java and p4/UnamedModuleDummyApp. I see what this istrying to do, which is to test a RMI server from an unnamed module. Itinstantiates p3.Server, which resides in a named module, but exports it froman unnamed module.So I think there's some tension here. There's subclassing among the clients that attempts to avoid duplication, which is good, but it doesn't truly seem to be testing clients in different modules and in an unnamed module. Theserver, on the other hand, does seem to be testing things properly indifferent modules, at the cost of duplicating all the code into another classthat resides in a different (unnamed) module.I'm not entirely sure what the best approach is here. Ideally you'd want tohave a single implementation of client, server + remote interface, andapplication, and compile each of them once. Then since you're invoking a new JVM for each test, invoke it with different options to put the client, or the server, or the app, into modules, or onto the classpath (to get into an unnamed module). You might have to copy compiled class files to different locations so that different classes can be either on the classpath or in anamed module without causing any conflicts.I'm willing to entertain some simplifications here as well. It might besufficient to deal with just clients and servers in different/unnamedmodules, and not worry about putting the application into different modules.That should reduce the number of cases to cover. s'marks On 2/29/16 10:05 PM, Felix Yang wrote:Ping... -Felix On 2016/2/24 14:06, Felix Yang wrote:Hi all, please review the new tests to use RMI in module world. Webrev: http://cr.openjdk.java.net/~xiaofeya/8078812/webrev.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8078812 Thanks, Felix
