Plugin "tester" refactoring

sormuras commented 7 years ago

Overview

This is a "tester"-centric design issue that will provide an answer to "How to test JPMS projects in and out".

The description is work-in-progress...

Module-path mode

java -ea 
--module-path
  target/test/exploded       // integration.pro and other real module-based "tests" apps
  target/pro/plugins/tester  // contains the `junit.platform.console.standalone` module
-m junit.platform.console.standalone/org.junit.platform.console.ConsoleLauncher <options>

Class-path mode

java -ea
-classpath
  target/test/exploded/com.github.forax.pro.api
  target/test/exploded/com.github.forax.pro.helper
  [...]
  target/pro/plugins/tester/junit.platform.console.standalone.jar
org.junit.platform.console.ConsoleLauncher <options>

Resources

forax commented 7 years ago

Do you really need a class-path mode ? i had the same issue with by example the compiler plugin and solve it by not supporting the class-path mode because you can use the module-fixer to transform a non modular jar to a modular jar.

The main issue i see with the classpath mode is that it won't read the module-info so the test specific dependencies will not be read.

sormuras commented 7 years ago

Do you really need a class-path mode ?

Maybe not. The module-path mode seem to work out-of-box even with the currently disabled ConfigsTests class:

java -ea 
--module-path target/test/exploded;target/pro/plugins/tester 
--module junit.platform.console.standalone/org.junit.platform.console.ConsoleLauncher 
   --scan-classpath target/test/exploded/com.github.forax.pro.api  | ... only needed to find
   --classpath target/test/exploded/com.github.forax.pro.api       |  the *Tests via file system
.
+-- JUnit Jupiter [OK]
| +-- CmdLineTests [OK]
| | +-- toStringWithoutArguments() [OK]
| | +-- toStringWithSingleArgument() [OK]
| | '-- toStringWithMultipleArguments() [OK]
| '-- ConfigsTests [OK]
|   '-- newRootIsNotNull() [OK]
'-- JUnit Vintage [OK]

Do you know an elegant way how find "tests" in a module (path)?

sormuras commented 7 years ago

The static Pro class initializer is having troubles finding dependencies when resolving dynamic plugins, like runner:

| | |              Caused by: java.lang.module.FindException: Module com.github.forax.pro.api not found, required by com.github.forax.pro.plugin.runner
| | |                   at java.base/java.lang.module.Resolver.findFail(Resolver.java:947)
| | |                   at java.base/java.lang.module.Resolver.resolve(Resolver.java:211)
| | |                   at java.base/java.lang.module.Resolver.resolve(Resolver.java:158)
| | |                   at java.base/java.lang.module.Configuration.resolve(Configuration.java:361)
| | |                   at java.base/java.lang.module.Configuration.resolve(Configuration.java:192)
| | |                   at com.github.forax.pro.api.impl.Plugins.findDynamicPlugins(Plugins.java:30)
| | |                   at java.base/java.util.stream.ReferencePipeline$7$1.accept(ReferencePipeline.java:269)
| | |                   at java.base/java.util.Iterator.forEachRemaining(Iterator.java:133)
| | |                   at java.base/java.util.Spliterators$IteratorSpliterator.forEachRemaining(Spliterators.java:1801)
| | |                   at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484)
| | |                   at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)
| | |                   at java.base/java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:151)
| | |                   at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:174)
| | |                   at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
| | |                   at java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:430)
| | |                   at com.github.forax.pro.api.impl.Plugins.getAllPlugins(Plugins.java:52)
| | |                   at com.github.forax.pro.Pro.<clinit>(Pro.java:96)
| | |                   ... 36 more

I spawned a new VM from within tester with the following arguments:

java
-ea
--module-path
target\test\exploded;target\main\exploded;target\pro\plugins\tester
--module
junit.platform.console.standalone/org.junit.platform.console.ConsoleLauncher
--details=verbose
--scan-classpath
target\test\exploded\com.github.forax.pro.helper
--scan-classpath
target\test\exploded\integration.pro
--scan-classpath
target\test\exploded\com.github.forax.pro.api
--classpath
target\test\exploded\com.github.forax.pro.helper
--classpath
target\test\exploded\integration.pro
--classpath
[...]
--classpath
target\main\exploded\com.github.forax.pro.api
--classpath
target\main\exploded\com.github.forax.pro

To reproduce, remove @Disabled from https://github.com/forax/pro/compare/master...sormuras:tester_spawn_vm#diff-73baa06e83ee4e99b3dcd3b32aa544beR12

sormuras commented 7 years ago

See https://github.com/forax/pro/compare/master...sormuras:tester_spawn_vm for work-in-progress code...

forax commented 7 years ago

External plugins are loaded dynamically by Pro, so they should not be part of the module-path. The problem here seems to be that the plugin tester and the command run by the plugin are the same module.

Also, i believe that each module should be tested in isolation, otherwise several test modules will see each other

forax commented 7 years ago

Christian, i've implemented a way to specify that a property of the conf depends on another one (see MutableConf.derive) and i've changed the conf of tester to use derive().

So, now, every paths set by default depends on pro.currentDir, so if pro.currentDir change, all properties are changed accordingly (technically, it's implemented the other way around, when you ask for the value of a property, it follows the derivation tree (a bunch of lambdas) to pro.currentDir).

Pro.local() is now easier to implement, it changes pro.currentDir.

sormuras commented 7 years ago

Rebased and the configuration aspect feels better now.

sormuras commented 7 years ago

Release pro-9- b167 -linux.tar.gz for the Travis CI build? Or are more changes on their way? ;)

forax commented 7 years ago

No, 2 bugs,

when building the external plugins (runner and tester), javac was using the the class of the bootstrap pro and not the newly compiled class (--upgrade-module-path) fix that issue
the packager was not using derive() so tester and runner were not packaged correctly

sormuras commented 7 years ago

Now, every module tests are executed on their own "run".

https://travis-ci.org/sormuras/pro/builds/227324876#L406

Still java.lang.NoClassDefFoundError: com/github/forax/pro/Pro ... I guess, the class loading facility in JUnit 5 needs some modular love.

forax commented 7 years ago

I believe it's because the test are loaded using an URLClassLoader so instead of loading the class inside the module, junit load the class as if it was declared in the classpath so the module-info is not read, so the requires is not resolved.

I wonder if there is a way to load a test from the current classloader (the one of the application). That's why i was saying that you may have to use the other API, not the command line launcher.

sormuras commented 7 years ago

Ah! Makes total sense!

Do you know an elegant way how find "tests" in a module (path)?

I'll start by manually selecting well-known classes. But (JUnit) should provide a "--scan-module" option.

sormuras commented 7 years ago

One last comment regarding the ConsoleLauncher: when the user provides an own Launcher-Launcher, it works. Because now the module-info is read and resolved: https://github.com/sormuras/application-junit5-jdk9-demo/blob/pro/build.pro#L15

forax commented 7 years ago

Implementing --scan--module is not hard, You create a moduleFinder like this [1] so you get one ModuleReference by module, with open() [2], you get a ModuleReader. On a ModuleReader, list() allow you to get all files inside a module (so all .class file), and either open() get you an InputStream to read the classfile without loding it, otherwise, you can load it by getting the ClassLoader of a Module (getClassLoader()) and loading the class (with loadClass())

[1] https://github.com/forax/pro/blob/master/plugins/tester/src/main/java/com.github.forax.pro.plugin.tester/com/github/forax/pro/plugin/tester/TesterPlugin.java#L57 [2] http://download.java.net/java/jdk9/docs/api/java/lang/module/ModuleReference.html#open--

forax commented 7 years ago

and i have no idea what a Launcher-Launcher is ?

sormuras commented 7 years ago

and i have no idea what a Launcher-Launcher is ?

A class like https://github.com/sormuras/application-junit5-jdk9-demo/blob/pro/src/main/java/integration/integration/ConsoleLauncherIntegration.java that loops-through the arguments to the ConsoleLauncher ... residing in the test module.

sormuras commented 7 years ago

About to move to the Launcher API. Only need to select the classes like you described above ... around here : https://github.com/forax/pro/compare/master...sormuras:tester_spawn_vm#diff-bfecff36cc9fd630a43b73ccef32a30cR161

And some major cleanup ... and re-implement (or publish) the JUnit 5 TreePrintingListener, as it is not part of the Launcher API, but the console module.

forax commented 7 years ago

I would prefer that you separate the change on the ModuleFixerPlugin, (and i'm not sure the second change is even right, "" as a special meaning in the analysis)

In TesterPlugin, Can you avoid to declare the local variable 'final', it make the code harder to read for littel meaning given that since java 8 local are effectively final by default shorten() should be a private method, a lambda does not seem necessary here (you can use it as a method reference if needed).

In CmdLine, it acts as a builder so getArgument() should copy the arguments otherwise, if you call add after getArguments() it will change the returned list. And perhaps toArgumentList() is a better name (more like a builder)

Other parts of the code seems fine.

I wonder if it's not better if we use pull request for discussing even if we accept our own request at the end ?

sormuras commented 7 years ago

I wonder if it's not better if we use pull request for discussing even if we accept our own request at the end?

Sure. I can file a PR -- but I thought that the current work in progress branch was not ready for a PR. Will change the git history here (squash, rebase, force-push) from time to time and when the features implemented here are 90% done, I'll create a PR.

The changes to CmdLine and ModuleFixerPlugin are reverted - because no longer needed. The shorten function is now a static method in FileHelper.

I'm still struggling to load the test classes via the applications class loader. See https://travis-ci.org/sormuras/pro/builds/227434803#L317 for the stack trace and https://github.com/forax/pro/compare/master...sormuras:tester_spawn_vm#diff-bfecff36cc9fd630a43b73ccef32a30cR107 for my initial attempt to load a test class.

sormuras commented 7 years ago

I tried to enhance the jshell environment via /env -module-path target/test/exploded [...] as the first command in file test.pro, but that didn't work out.

I wonder if there is a way to load a test from the current classloader (the one of the application).

When java/jshell/pro calls the "tester" plugin execute method, the compiled modules like "integration.pro" are not part of the application module path, right? I could again spawn a vm and configure the module path to the current needs, i.e. include the main and test exploded root paths, but then I need a handle into the module that contains the test classes. Like the Launcher-Launcher...

...seems like I'm running here in circles, again. :(

sormuras commented 7 years ago

Mh, the dynamically loaded modules like runner and tester are not loaded at start of pro build test.pro. Perhaps, I have to add the modules with the test classes in a similar way, like Pro does in its static init code block?

pro build test.pro
NOTE: Picked up the following options via JDK_JAVA_OPTIONS:
  --list-modules
module aether.api@1.0.1
module aether.connector.basic@1.0.1
module aether.impl@1.0.1
module aether.spi@1.0.1
module aether.transport.classpath@1.0.1
module aether.transport.file@1.0.1
module aether.transport.http@1.0.1
module aether.transport.wagon@1.0.1
module aether.util@1.0.1
module com.github.forax.pro@0.9
module com.github.forax.pro.aether@0.9
module com.github.forax.pro.aether.fakeguava@1.0
module com.github.forax.pro.api@0.9
module com.github.forax.pro.daemon@0.9
module com.github.forax.pro.daemon.imp@0.9
module com.github.forax.pro.helper@0.9
module com.github.forax.pro.main@0.9
module com.github.forax.pro.plugin.compiler@0.9
module com.github.forax.pro.plugin.convention@0.9
module com.github.forax.pro.plugin.linker@0.9
module com.github.forax.pro.plugin.modulefixer@0.9
module com.github.forax.pro.plugin.packager@0.9
module com.github.forax.pro.plugin.resolver@0.9
module com.github.forax.pro.plugin.uberpackager@0.9
module com.github.forax.pro.uberbooter@0.9
module com.github.forax.pro.ubermain@0.9
module commons.lang@3.5
module commons.logging.api@1.1
module httpclient@4.2.6
module httpcore@4.2.5
module java.activation@9-ea
module java.base@9-ea
module java.compiler@9-ea
module java.corba@9-ea
module java.datatransfer@9-ea
module java.desktop@9-ea
module java.instrument@9-ea
module java.jnlp@9-ea
module java.logging@9-ea
module java.management@9-ea
module java.management.rmi@9-ea
module java.naming@9-ea
module java.prefs@9-ea
module java.rmi@9-ea
module java.scripting@9-ea
module java.se@9-ea
module java.se.ee@9-ea
module java.security.jgss@9-ea
module java.security.sasl@9-ea
module java.smartcardio@9-ea
module java.sql@9-ea
module java.sql.rowset@9-ea
module java.transaction@9-ea
module java.xml@9-ea
module java.xml.bind@9-ea
module java.xml.crypto@9-ea
module java.xml.ws@9-ea
module java.xml.ws.annotation@9-ea
module javafx.base@9-ea
module javafx.controls@9-ea
module javafx.deploy@9-ea
module javafx.fxml@9-ea
module javafx.graphics@9-ea
module javafx.media@9-ea
module javafx.swing@9-ea
module javafx.web@9-ea
module jdk.accessibility@9-ea
module jdk.attach@9-ea
module jdk.charsets@9-ea
module jdk.compiler@9-ea
module jdk.crypto.cryptoki@9-ea
module jdk.crypto.ec@9-ea
module jdk.crypto.mscapi@9-ea
module jdk.deploy@9-ea
module jdk.deploy.controlpanel@9-ea
module jdk.dynalink@9-ea
module jdk.editpad@9-ea
module jdk.hotspot.agent@9-ea
module jdk.httpserver@9-ea
module jdk.incubator.httpclient@9-ea
module jdk.internal.ed@9-ea
module jdk.internal.jvmstat@9-ea
module jdk.internal.le@9-ea
module jdk.internal.opt@9-ea
module jdk.internal.vm.ci@9-ea
module jdk.jartool@9-ea
module jdk.javadoc@9-ea
module jdk.javaws@9-ea
module jdk.jcmd@9-ea
module jdk.jconsole@9-ea
module jdk.jdeps@9-ea
module jdk.jdi@9-ea
module jdk.jdwp.agent@9-ea
module jdk.jfr@9-ea
module jdk.jlink@9-ea
module jdk.jshell@9-ea
module jdk.jsobject@9-ea
module jdk.jstatd@9-ea
module jdk.localedata@9-ea
module jdk.management@9-ea
module jdk.management.agent@9-ea
module jdk.naming.dns@9-ea
module jdk.naming.rmi@9-ea
module jdk.net@9-ea
module jdk.pack@9-ea
module jdk.packager@9-ea
module jdk.packager.services@9-ea
module jdk.plugin@9-ea
module jdk.plugin.dom@9-ea
module jdk.plugin.server@9-ea
module jdk.policytool@9-ea
module jdk.rmic@9-ea
module jdk.scripting.nashorn@9-ea
module jdk.scripting.nashorn.shell@9-ea
module jdk.sctp@9-ea
module jdk.security.auth@9-ea
module jdk.security.jgss@9-ea
module jdk.snmp@9-ea
module jdk.unsupported@9-ea
module jdk.xml.bind@9-ea
module jdk.xml.dom@9-ea
module jdk.xml.ws@9-ea
module jdk.zipfs@9-ea
module maven.aether.provider@3.3.9
module maven.artifactfat@3.3.9
module maven.builder.support@3.3.9
module maven.modelfat@3.3.9
module oracle.desktop@9-ea
module oracle.net@9-ea
module org.json@20160810
module org.objectweb.asm.all.debug@6.0_ALPHA2
module plexus.interpolation@1.24
module plexus.utils@3.0.24

forax commented 7 years ago

--list-modules gives you the list of the module statically (at boot time) loaded by the VM runner and tester are loaded dynamically thus not part of the modules listed by --list-modules.

for your problem, i think you should load a test module dynamically like this https://github.com/forax/pro/blob/master/src/main/java/com.github.forax.pro.api/com/github/forax/pro/api/impl/Plugins.java#L26 and then load the test from the classloader you have just created

sormuras commented 7 years ago

I just bookmarked that method moments before your comment. ;)

The classloader then gets the single test module and all modules from main modules from target/main/artifact -- except the test module one -- as roots, right?

forax commented 7 years ago

yes !

sormuras commented 7 years ago

Created a new branch at https://github.com/forax/pro/compare/master...sormuras:tester_module_mode that is very close to turn into a pull request. The loaded test classes are registered at the JUnit Platform Launcher ... but not yet executed. Need to debug it further.

See the output at https://travis-ci.org/sormuras/pro/builds/227741212#L313 :

Executing tests in path: target/test/exploded/integration.pro
Executing tests in module { 
  name: integration.pro, 
   [ junit.platform.commons (@1.0.0-M4),
     opentest4j (@1.0.0-M2),
     junit.jupiter.api (@5.0.0-M4),
  mandated java.base (@9-ea),
     com.github.forax.pro (@0.9)]
}

selectClass(class integration.pro.ProTests)

Test run finished after 1 ms
[         2 containers found      ]
[         0 containers skipped    ]
[         2 containers started    ]
[         0 containers aborted    ]
[         2 containers successful ]
[         0 containers failed     ]
[         0 tests found           ]
[         0 tests skipped         ]
[         0 tests started         ]
[         0 tests aborted         ]
[         0 tests successful      ]
[         0 tests failed          ]

sormuras commented 7 years ago

The loaded and registered test classes (like ProTests) are pruned from the test plan before execution because JUnit fails to recognize the @org.junit.jupiter.api.Test annotation at the methods.

Here is the relevant logging snippet. The first block is the type JUnit looks for, the second is the actual annotation instance present at the ProTests.list() method:

annotationType=interface org.junit.jupiter.api.Test 
  @jdk.internal.loader.Loader@149494d8

candidate.annotations=
 [ 
  annotation=@org.junit.jupiter.api.Test() 
  annotation.getClass()=class com.sun.proxy.$Proxy41
  annotation.annotationType()=interface org.junit.jupiter.api.Test
    @jdk.internal.loader.Loader@7bab3f1a]

Can I fix this within pro (there are still the compiler warning at https://travis-ci.org/forax/pro#L170) or the tester plugin (via module-path cleanup)?

Or is it a bug in JUnit to rely on reflection in: https://github.com/junit-team/junit5/blob/master/junit-platform-commons/src/main/java/org/junit/platform/commons/util/AnnotationUtils.java#L156

sormuras commented 7 years ago

Solved via #33 and #35

forax / pro