testng-team / testng

TestNG testing framework
https://testng.org
Apache License 2.0
1.99k stars 1.02k forks source link

Implement Guice scopes: @TestSuiteScope, @TestClassScope, @TestMethodScope #2605

Open vlsi opened 3 years ago

vlsi commented 3 years ago

TestNG-Guice integration is helpful, however, there are issues:

  1. It is hard to create "global" objects. For instance, if I need to create a database connection pool and reuse it in several test classes, I have to resort to static fields or something like that. The same goes for web browsers, etc, etc.

    Of course, storing global objects in statics does not sound too hard, however, it cerates unnecessary friction where DI engines (and JSR-330) do support scoping (e.g. @javax.inject.Singleton)

  2. There's no clear API for "per-test suite", "per-test class", "per-test method" scoping. In other words, I want some of the objects to be re-created for each test (they depend on test method name), and TestNG provides no facility for that. Well, currently Guice modules are re-created for each test class, so storing suite-wise information is hard.

    See https://github.com/cbeust/testng/issues/398, https://github.com/cbeust/testng/issues/2343

  3. The worse, there's no scope control. In other words, if I accidentally inject a test-specific object into a global one, I would have bad behavior. If the scoping was explicit, then extra verifications could be added like "don't inject test-scoped instances into suite-scoped ones"

    The execution flow is hard to reason. I've created https://docs.google.com/document/d/1_Y_jCq6hrEj3n6_fuPBJOAvpo625c_Y1TtO_fncrOEY/edit# with links, however, it would be great if there was an easy way to share or unshare objects between tests.

  4. Listener execution order is not defined, so listeners do not really suit for maintaining DI scopes 🤷

  5. ParentModule feature is too limited, and it is hard to use: it is declared in a completely different manner than @Guice(...), and it imposes unrelated binding to ITestContext and testClass via Module createModule(ITestContext context, Class<?> testClass). The idea of using DI is that one should declare which dependencies they need, and the idea of requiring users to implement createModule(ITestContext, Class<?>) when they need neither of the parameters looks weird.

  6. I remember @juherr wanted to remove strong dependency on Guice. I agree it might be worth doing some time in the future, I do not think it is easily resolvable. Apparently, JSR-330 did not work that well for both Spring and Guice, so making yet-another-jsr-330 for the sole purpose of "enabling users to plug their DI implementations" does not spark joy for me. I do not have a need to plug an alternative DI engine to TestNG, so I do not want to spend my time on "removing strong dependency on Guice".


A specific use-case that bothers me is I want to inject data-generation helpers to test classes (so each test class would get the relevant data to work with), and I want to have a "per-suite" service that would know of all the requested data, so it could upload it to the system before suite starts executing.

I would like to add Guice scopes support to TestNG, so test code could request "per-suite" (I need it for common data and DB connections), "per-test class" (I need so each class generates its own input data), or "per-method" (I do not need it yet, however, it might be useful for parameter injection).


So my suggestion is to forget "removing strong dependency on Guice" for a while, and add support for Guice-based scopes to TestNG as if Guice was the only DI supported by TestNG. That would make the implementation much easier, and, if someone needs another DI engine they can either make bridges to Guice or contribute "removing strong dependency on Guice" feature.

WDYT?

vlsi commented 3 years ago

It looks like DI does not have to be JSR-330 or something, and the API should be more like T getInstance(Class klass), void injectValues(Object instance) + scope events (so injector could adapt accordingly)

vlsi commented 3 years ago

In other words, testng does not need to know @*.Inject or supply its own @Inject annotations or even depend on them.

juherr commented 3 years ago

Listeners are supposed to be created before the test run step. That's why we need specific consideration for them.

The goal behind "removing strong dependency on Guice" is more "As a User, I want to override the default Guice implementation" which is more/less what you are asking for. Another goal is trying to reduce the current complexity of the test engine.

There is an API that allows users to intercept object creation and listeners that notify about the scope. I think it should be enough to implement whatever you want before increasing the complexity. If something is missing in API, I think improving it will be a good first step.

vlsi commented 3 years ago

There is an API that allows users to intercept object creation and listeners that notify about the scope. I think it should be enough to implement whatever you want before increasing the complexity.

Are you up to a Zoom or whatever discussion? Any preferences on timeframes? Here are the timeframes that work for me the best: https://doodle.com/mm/vladimirsitnikov/30min-meeting

There's JSR 365: Contexts and Dependency Injection for Java 2.0 that declares APIs for object creation, bean registration, interceptors. However, the JSR is quite sophisticated, and in practice, it is not implemented by Guice/Spring.

vlsi commented 3 years ago

If something is missing in API, I think improving it will be a good first step.

I think the minimal API is

vlsi commented 3 years ago

The next issue is that TestNG exposes API to override annotation values (e.g. IAnnotationTransformer), and the transformed values will likely be ignored by the DI engines. For instance, Guice uses java.lang.Class APIs to get annotations, so it won't observe the output of IAnnotationTransformer