search

spockframework / spock

The Enterprise-ready testing and specification framework.
https://spockframework.org
Apache License 2.0
3.55k stars 468 forks source link

IGlobalExtension start() and stop() are run excess amount of times #1240

Open Net-burst opened 3 years ago

Net-burst commented 3 years ago

Issue description

IGlobalExtension#start() and stop() are run during SpockEngine test collection even if there are no test classes for Spock to collect. This leads to start() and stop() being run even if there are no Spock tests to run. For example, if you run JUnit tests. This changed with Spock 2.0. Spock 1.3 doesn't exhibit this. This happens in due to this chain org.spockframework.runtime.SpockEngine#discover -> org.spockframework.runtime.RunContext#get -> org.spockframework.runtime.RunContext#start . Discover might not return anything for Spock to run, but extensions will be fire any way.

How to reproduce

Instead of posting huge snippets of code here, here's the repo with minimalistic Maven project to illustrate this issue: https://github.com/Net-burst/spock-junit5-runner I added console output for start(), stop() and visitSpec(). Please look at when Spock extension start, Spock extension stop and Spock extension start visitSpec appear.

In Maven scenarios this leads to extension code being executed needlessly several times:

So, apart from start() being executed 2 times (first for the surefire plugin, when there are no collected Spock specifications and them for the failsafe plugin), stop() is also being executed 2 times. Also IDK why is visitSpec() executed an excess amount of times.

Additional Environment information

Java: Coretto 11.0.8 Spock: 2.0-M4-groovy-3.0 Groovy: 3.0.6 IDEA: 2020.2.4 Ultimate OS: Linux (Ubuntu 20.04) / Windows 10

Java/JDK

openjdk 11.0.8 2020-07-14 LTS
OpenJDK Runtime Environment Corretto-11.0.8.10.1 (build 11.0.8+10-LTS)
OpenJDK 64-Bit Server VM Corretto-11.0.8.10.1 (build 11.0.8+10-LTS, mixed mode)

Groovy version

Groovy 3.0.6 running from IDEA as Maven dependency

Build tool version

Apache Maven

Apache Maven 3.6.3 (cecedd343002696d0abb50b32b541b8a6ba2883f)
Maven home: C:\Users\NetBurst\AppData\Local\JetBrains\Toolbox\apps\IDEA-U\ch-0\202.8194.7\plugins\maven\lib\maven3
Java version: 11.0.8, vendor: Amazon.com Inc., runtime: C:\Users\NetBurst\.jdks\corretto-11.0.8
Default locale: en_US, platform encoding: UTF-8
OS name: "windows 10", version: "10.0", arch: "amd64", family: "windows"

Operating System

Tested on both Ubuntu 20.04 and Windows 10

IDE

IDEA 2020.2.4 Ultimate

Build-tool dependencies used

Apache Maven

    <dependencies>
        <dependency>
            <groupId>org.spockframework</groupId>
            <artifactId>spock-core</artifactId>
            <version>2.0-M4-groovy-3.0</version>
        </dependency>
        <dependency>
            <groupId>org.junit.jupiter</groupId>
            <artifactId>junit-jupiter-api</artifactId>
            <version>5.7.0</version>
            <scope>test</scope>
        </dependency>
        <dependency>
            <groupId>org.junit.jupiter</groupId>
            <artifactId>junit-jupiter-engine</artifactId>
            <version>5.7.0</version>
            <scope>test</scope>
        </dependency>
    </dependencies>

    <build>
        <plugins>
            <plugin>
                <groupId>org.codehaus.gmavenplus</groupId>
                <artifactId>gmavenplus-plugin</artifactId>
                <version>1.11.0</version>
                <configuration>
                    <parallelParsing>true</parallelParsing>
                </configuration>
                <executions>
                    <execution>
                        <goals>
                            <goal>compile</goal>
                            <goal>compileTests</goal>
                        </goals>
                    </execution>
                </executions>
            </plugin>
            <plugin>
                <groupId>org.apache.maven.plugins</groupId>
                <artifactId>maven-surefire-plugin</artifactId>
                <version>2.22.2</version>
                <configuration>
                    <includes>**/*Test.java</includes>
                </configuration>
            </plugin>
            <plugin>
                <groupId>org.apache.maven.plugins</groupId>
                <artifactId>maven-failsafe-plugin</artifactId>
                <version>2.22.2</version>
                <configuration>
                    <includes>**/*Spec.java</includes>
                </configuration>
                <executions>
                    <execution>
                        <goals>
                            <goal>integration-test</goal>
                            <goal>verify</goal>
                        </goals>
                    </execution>
                </executions>
            </plugin>
        </plugins>
    </build>
kriegaex commented 3 years ago

I did not clone your repo yet, just glanced at the Maven config and the log output. Some thoughts:

I am currently busy and just had a couple of minutes, so I cannot elaborate further for now or run and analyse your sample project. I might have time for that later this weekend, but chances are that someone else is faster and can provide a much more precise and clear answer.

Net-burst commented 3 years ago

Surefire and Failsafe run in two completely different Maven phases and also each in its own JVM, so it is to be expected that Spock is started once per JVM. So far, so unsurprising.

Yep, sure. But the issue is that during the surefire phase there were no test classes for Spock to collect. During Sputnik times, global extensions were run only if there were actual tests for Spock to run. And now we just run everything unconditionally. The only possibility to block Spock during the test phase is to switch to Gradle and filter out the Spock engine. But that's not the option unfortunately and Maven cannot do this :(

That Spock is started even though there are not Spock tests might be due to the fact that Spock 2.x implements its own JUnit 5 test engine, hence has to be started in order to determine if there are any tests to be run (chicken vs. egg problem, also called boot-strapping problem). Because a global extension is in fact global, it must be registered before any tests are being discovered, otherwise, it could be too late to initialize whatever global resources it wants to manage. The same goes for closing the extension.

Agree, but JUnit 5 engine has 2 distinct phases: discover() and execute(). Do we even need to run extensions during the discovery phase? Because we might not discover anything to run at all. I know that I'm talking from the perspective of my own use case, but still :)

I am currently busy and just had a couple of minutes, so I cannot elaborate further for now or run and analyse your sample project. I might have time for that later this weekend, but chances are that someone else is faster and can provide a much more precise and clear answer.

Thanks for your efforts!

kriegaex commented 3 years ago

the issue is that during the surefire phase there were no test classes for Spock to collect. During Sputnik times, global extensions were run only if there were actual tests for Spock to run. And now we just run everything unconditionally.

Well, we are in a way "comparing apples and pears" here, as Germans like to put it. Sputnik extends Runner, i.e. by definition is limited to doing whatever it does during text execution rather than discovery phase. I consider this a limitation in Spock 1.3 because you cannot hook into discovery with a global extension, which might be desirable. But as you said, in Spock 2 it is different:

That Spock is started even though there are not Spock tests might be due to the fact that Spock 2.x implements its own JUnit 5 test engine (...)

Agree, but JUnit 5 engine has 2 distinct phases: discover() and execute(). Do we even need to run extensions during the discovery phase? Because we might not discover anything to run at all. I know that I'm talking from the perspective of my own use case, but still :)

In JUnit 5 and therefore also in Spock 2 you can load tests from sources other than class files stored on the local file system or inside JAR files. It is even possible to synthesise them on the fly (i.e. to create them dynamically) without any class file representation. Imagine for example that a global extension would open a database or other server connection during discovery phase and make tests available which would not even be detected and consequently not run otherwise. I very much like this option to be available to global extensions, even though you might in your use case find it annoying or disturbing.

A possible future Spock improvement might be for global extensions to be able to indicate in which of the two phases they wish to be started so as to avoid starting execution-only global extensions in the discovery phase. Execution could even be the default IMO, i.e. extension developers would have to explicitly indicate the extension needs "super powers" which now it automatically possesses.

kriegaex commented 3 years ago

BTW, if you are looking for a workaround for excluding a direct (not a transitive) dependency in a Maven plugin configuration, you may want to check my StackOverflow answer here and/or my related comment in MNG-6222. That way you might be able to completely avoid running Spock the otherwise auto-detected Spock engine at all for Surefire or Failsafe in a specific module.

Net-burst commented 3 years ago

A possible future Spock improvement might be for global extensions to be able to indicate in which of the two phases they wish to be started so as to avoid starting execution-only global extensions in the discovery phase. Execution could even be the default IMO, i.e. extension developers would have to explicitly indicate the extension needs "super powers" which now it automatically possesses.

This one might really be a very nice feature to have, yeah. My main concern was more about a lack of clarity on behavior change.

BTW, if you are looking for a workaround for excluding a direct (not a transitive) dependency in a Maven plugin configuration, you may want to check my StackOverflow answer here and/or my related comment in MNG-6222. That way you might be able to completely avoid running Spock the otherwise auto-detected Spock engine at all for Surefire or Failsafe in a specific module.

Too ugly, unfortunately. That's some sacred knowledge there so I don't want to pull that into a production project. For the time being I just moved my logic to visitSpec() (which is also being run more times than expected) and added a flag to indicate that I already did initialization. And stop() just relies on that flag. Easy.

leonard84 commented 3 years ago

I did some tests the double stop() is caused by one time being called by the JUnit Platform and a second time via a shutdown hook that functions as a backstop. In Spock 1.x the hook was the only way, since JUnit 4 didn't offer something like a global stop hook to its runners. We might think about removing the shutdown hook, since the platform now offers the necessary hooks.

As for visitSpec being called multiple times, this seems to be caused by surefire: Stacktrace 1:

...
        at org.spockframework.runtime.SpockEngineDiscoveryPostProcessor.postProcessEngineDescriptor(SpockEngineDiscoveryPostProcessor.java:17)
        at org.spockframework.runtime.SpockEngine.discover(SpockEngine.java:31)
        at org.junit.platform.launcher.core.EngineDiscoveryOrchestrator.discoverEngineRoot(EngineDiscoveryOrchestrator.java:103)
        at org.junit.platform.launcher.core.EngineDiscoveryOrchestrator.discover(EngineDiscoveryOrchestrator.java:85)
        at org.junit.platform.launcher.core.DefaultLauncher.discover(DefaultLauncher.java:92)
        at org.junit.platform.launcher.core.DefaultLauncher.discover(DefaultLauncher.java:67)
        at org.apache.maven.surefire.junitplatform.TestPlanScannerFilter.accept(TestPlanScannerFilter.java:56)
        at org.apache.maven.surefire.util.DefaultScanResult.applyFilter(DefaultScanResult.java:102)
        at org.apache.maven.surefire.junitplatform.JUnitPlatformProvider.scanClasspath(JUnitPlatformProvider.java:146)
        at org.apache.maven.surefire.junitplatform.JUnitPlatformProvider.invoke(JUnitPlatformProvider.java:127)
        at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:377)
        at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:138)
        at org.apache.maven.surefire.booter.ForkedBooter.run(ForkedBooter.java:465)
        at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:451)

and Stacktrace 2:

...
        at org.spockframework.runtime.SpockEngineDiscoveryPostProcessor.postProcessEngineDescriptor(SpockEngineDiscoveryPostProcessor.java:17)
        at org.spockframework.runtime.SpockEngine.discover(SpockEngine.java:31)
        at org.junit.platform.launcher.core.EngineDiscoveryOrchestrator.discoverEngineRoot(EngineDiscoveryOrchestrator.java:103)
        at org.junit.platform.launcher.core.EngineDiscoveryOrchestrator.discover(EngineDiscoveryOrchestrator.java:85)
        at org.junit.platform.launcher.core.DefaultLauncher.discover(DefaultLauncher.java:92)
        at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:75)
        at org.apache.maven.surefire.junitplatform.JUnitPlatformProvider.invokeAllTests(JUnitPlatformProvider.java:154)
        at org.apache.maven.surefire.junitplatform.JUnitPlatformProvider.invoke(JUnitPlatformProvider.java:127)
        at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:377)
        at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:138)
        at org.apache.maven.surefire.booter.ForkedBooter.run(ForkedBooter.java:465)
        at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:451)

@marcphilipp do you know of any such issues? From what I see surefire is calling org.junit.platform.launcher.core.DefaultLauncher#discover(org.junit.platform.launcher.LauncherDiscoveryRequest) first and then it calls org.junit.platform.launcher.core.DefaultLauncher#execute(org.junit.platform.launcher.LauncherDiscoveryRequest, org.junit.platform.launcher.TestExecutionListener...) instead of org.junit.platform.launcher.core.DefaultLauncher#execute(org.junit.platform.launcher.TestPlan, org.junit.platform.launcher.TestExecutionListener...) so it is basically calling discover twice, discarding the TestPlan returned by the first call.

marcphilipp commented 3 years ago

IIRC Surefire calls discover() for every potential test class prior to executing it. Is there a way to defer calling IGlobalExtension until SpockEngine execute is called?

leonard84 commented 3 years ago

Well, since the extensions can also exclude Specs/Features from being executed, change method orders, define name and so on, they have to run during the discover phase. All the visit* methods of global and annotation-driven extensions are run during the discovery so that the nodes are ready to execute, the real expensive stuff should be done with the interceptors. It is unfortunate, that the initial nodes are being discarded and that the whole work has to be performed a second time.

If I understand the JUnit Platform API right, surefire should use discover to get the TestPlan and then execute it instead of discarding it and starting another discovery with a LauncherDiscoveryRequest. Or, is there a limitation in the platform that forces them to execute via a new LauncherDiscoveryRequest, e.g. to support forks?

marcphilipp commented 3 years ago

I see, thanks for the explanation! Yes, I think that should work by changing the code in Surefire, assuming they're willing to drop support for older versions of JUnit Platform that didn't yet have the execute(TestPlan, ...) method.

Net-burst commented 3 years ago

From the end-user perspective, the whole global extension stuff should look like this:

At least that's how it aligns for me. My rationale is this: many projects split test scopes between Maven test (Surefire, only unit tests) and Maven verify (Failsafe, functional/integration tests). And a lot of projects use regular JUnit for unit tests and Spock for functional/integration tests. So, it's not desirable to run the Spock setup code if there is nothing for it to actually run. Adding new methods purely for the discovery phase will allow us to leave the same behavior between 1.3 and 2.0.

leonard84 commented 3 years ago

With the current surefire implementation you'd get spammed with discoveryStart/discoveryStop, since surefire actually launches a separate discovery request for every individual class, and then again one last time a combined request for the execution.

We could think about adding phase specific methods, but to keep them backwards compatible discoveryStart would have to call start and executionStop would have to call stop.

So, it's not desirable to run the Spock setup code if there is nothing for it to actually run.

While I understand the sentiment, in most cases it is a non issue. If you have special extensions that do heavy stuff, I'd suggest to simply defer the initialization as you've already done.

And a lot of projects use regular JUnit for unit tests and Spock for functional/integration tests.

Interesting, what is the reason for the split? Much of Spocks functionality is especially useful for unit testing, e.g. mocking and data-driven tests. So unless they are legacy tests, I don't see a reason to prefer JUnit tests over Spock tests for unit testing.

leonard84 commented 3 years ago

I've created an issue in the surefire bug tracker https://issues.apache.org/jira/browse/SUREFIRE-1864

kriegaex commented 3 years ago

a lot of projects use regular JUnit for unit tests and Spock for functional/integration tests

I would be very surprised if this was true for the same reasons as Leonard. Pray tell where you got your data about that "lot of projects" from. Why would anyone exclude the very type of tests profiting the most from using Spock? What is the rationale behind such a decision?

CamilYed commented 3 years ago

Is it possible to conditionally add a global extension? I would like to off or on some extensions by SpockConfig.groovy file, or env variable.

leonard84 commented 3 years ago

Hi @CamilYed welcome, please don't hijack an issue for off-topic question. You can use one of our communication channels

asteiner-psh commented 1 year ago

Any movement on this issue? I ran into the stop() runs multiple times on Spock 2 when run via JUnit Platform.