BeforeAll hooks are executed before dependency injection

[BenjaminTanguay]

👓 What did you see?

When a BeforeAll hook is defined, it is called before any dependencies are injected. This negates a big portion of the usefulness of the hook.

✅ What did you expect to see?

I expected the injection to be called and completed before any execution hook are invoked. It worked this way in Cucumber 7.0.0. Some change since that version broke this behavior.

📦 Which tool/library version are you using?

    <junit.version>5.10.0</junit.version>
    <cucumber.version>7.14.0</cucumber.version>
    <junit-platform-suite.version>1.10.0</junit-platform-suite.version>

The behavior was fine before I updated the dependency from:

    <junit.version>5.8.2</junit.version>
    <cucumber.version>7.0.0</cucumber.version>
    <junit-platform-suite.version>1.8.2</junit-platform-suite.version>

EDIT: Using the reproducer code, it seems that the behavior change happened between version 7.1.0 and 7.2.0.

🔬 How could we reproduce it?

https://github.com/BenjaminTanguay/cucumber-injection-hook-bug

Execute the reproducer.sh. It will run the failing code in both the current version and the older failing version.

The relevant elements are: resources/cucumber.properties test/java/com/example/RunCucumber.java test/java/com/example/hook/Hook.java test/java/com/example/injection/CucumberInjector.java test/java/com/example/injection/InjectionModule.java

[mpkorstanje]

Would you be able to narrow down in which version specifically this behavior changed?

[BenjaminTanguay]

I believe the behavior change occured between version 7.1.0 and 7.2.0

https://github.com/cucumber/cucumber-jvm/compare/v7.1.0...v7.2.0

I'm not an expert on this codebase but from the PRs mentionned in the 7.2.0 changelog, this one could be the source of the changed behavior:

https://github.com/cucumber/cucumber-jvm/pull/2432/files

[mpkorstanje]

Cheers! That helped!

The changes from #2432 moved the creation of the Injector. In v7.1.0 it used to be created in the constructor:

https://github.com/cucumber/cucumber-jvm/blob/ba275bf41bf83ec69549025775624ca5ecf3429c/guice/src/main/java/io/cucumber/guice/GuiceFactory.java#L17-L19

And since v7.2.0 it is created when the first test on a specific thread starts:

https://github.com/cucumber/cucumber-jvm/blob/fd1d7f1b849584e15fae4b1e0d3701603b00640e/guice/src/main/java/io/cucumber/guice/GuiceFactory.java#L70-L76

Unfortunately fixing this while retaining the functionality added in #2432 is rather difficult. ObjectFactory implementations are intended to create objects injected into scenarios and aren't designed to work out side of that scope. For example, each thread instantiates its own ObjectFactory instance. So there is no guarantee that injection of static fields happens only once or exclusively prior to test execution. This happens when executing in parallel on a ForkJoinPool.

One possible option would be to add implement something like the BeforeAllCallback from JUnit 5. When implemented by the GuiceFactory this could be used to create the Injector exactly once. And I suppose each scenario could then use Injector.createChildInjector() to avoid conflicts with other scenarios. But that's stretching my knowledge of Guice a bit.

A complicating factor here is that Cucumbers internals and state management is all rather adhoc. So implementing anything like JUnit 5s BeforeAllCallback and by extension JUnit5s extension system is anything but trivial.

If you need a more immediate solution, you may be better of lifting the v7.2 implementation of the Guice factory into your own project instead.

[BenjaminTanguay]

I traced the code a little bit and I'm sure this is probably a naive take but I don't see why it's impossible restoring the old behavior. I'll try and detail what I understood to see if I got something wrong.

As far as I understand it, the start method of the ObjectFactory is responsible for starting the injection no matter the framework used.

The supplier that provides this ObjectFactory is created in the build method of the Runtime object, and invoked before the runtime is started. This is logical: we first parse things before we start our tests.

The supplier that provides the ObjectFactory is invoked in the getRunner method of the CucumberExecutionContext class. This getRunner is called before three methods: beforeAllHooks, afterAllHooks, and runTestCase. That is to say that the ObjectMapperProvider is available before beforeAllHooks is called. In my mind, that means that it would be possible to add some code in the beforeAllHooks method to start the ObjectFactory.

The Runner class currently only invokes the start method of the ObjectFactory in the buildBackendWorlds method, as part of the runPickle method. Wouldn't it possible to have the Runner class invoke objectFactory.start() in the runBeforeAllHooks() it exposes? If not, then it could alternatively expose an inject method that is called by the portion of the CucumberExecutionContext class resposible for handling beforeAllHooks.

Where did I go wrong?

[mpkorstanje]

Fundamentally, I don't think it is impossible. But I do think the amount of work and complexity involved is significant. Especially for the time I'm currently able to dedicate to Cucumber.

To explain...

Currently the Runner, Backend and ObjectFactory implementations are effectively thread local.

https://github.com/cucumber/cucumber-jvm/blob/0a0f1730eded36c8367ea2e966acbf1fffdeb7dc/cucumber-junit-platform-engine/src/main/java/io/cucumber/junit/platform/engine/CucumberEngineExecutionContext.java#L58-L63

So for every thread, a new instance of the ObjectFactory is created. This means that the invocation of it's constructor isn't equivalent to a before all hook. It also means that there can be more than one ObjectFactory.

Wouldn't it possible to have the Runner class invoke objectFactory.start() in the runBeforeAllHooks() it exposes? If not, then it could alternatively expose an inject method that is called by the portion of the CucumberExecutionContext class resposible for handling beforeAllHooks.

No. The start/stop methods are invoked around a scenario. These are essential to keep state from leaking between scenarios.

It would be possible to add a startBeforeAll method instead that is invoked prior to the before all hooks.

But...

This would then require a way to ensure the Injector is only created once. And while it would be possible to create the Injector as a static variable, expecting people to do as part of an API is asking for problems.

A solution to that would be to provide startBeforeAll and start with a context object that can be used to store the Injector. And at this point we have re-invented the BeforeAllCallBack from JUnit 5.

Written from my phone. Apologies for any brevity.

[mpkorstanje]

That said looking at the GuiceFactory I do think that it is possible to instantiate the Injector eagerly.

Either when the from property is set:

https://github.com/cucumber/cucumber-jvm/blob/0a0f1730eded36c8367ea2e966acbf1fffdeb7dc/cucumber-guice/src/main/java/io/cucumber/guice/GuiceFactory.java#L31

Or when the from class is set

https://github.com/cucumber/cucumber-jvm/blob/0a0f1730eded36c8367ea2e966acbf1fffdeb7dc/cucumber-guice/src/main/java/io/cucumber/guice/GuiceFactory.java#L42

It would not be perfect because of the problems mentioned above, but I think it would at least restore the v7.1 behavior.

Would you mind looking into that possibility further?