@ExplicitParamInjection change in behavior

[snv]

I have two similar Test-Classes (in two seperate, but similar projects), one running with weld-junit5:2.0.2.Final (because it has to be compatible with the older API) and a newer Test wich runs with weld-junit5:4.0.1.Final.

These tests use jUnit5's @Nested-classes and additional extensions with parameter-resolvers.

In the older version it sufficed to put @ExplicitParamInjection on the surrounding test-class and it worked for all tests run.

In the newer version i apparently have to put the annotation on every @Nested-class, which is an unexpected and unwelcome bloat.

I would prefer the old behavior, or at least a hint in the annotation's javadoc and/or documentation.

[manovotn]

Hello,

the @ExplicitParamInjection annotation isn't said to be 'inherited' in any way so I am a bit surprised this was working in previous versions. There is only a very simple one-class test for this so we might have easily missed the fact that it previously scanned declaring class as well.
I suppose you could look at it the other way around and imagine a hierarchy of nested classes in which some require this annotation and others don't - with the approach you suggest, there would be no way to only make this option work for one test.
Not saying which one is better, just thinking aloud...

Note that one other way to enable this globally is a system property - https://github.com/weld/weld-testing/tree/master/junit5#explicit-parameter-injection
That way, you'd have it on for all tests (which may or may not be desirable) at all times.

I would prefer the old behavior, or at least a hint in the annotation's javadoc and/or documentation.

We should definitely at least mention it.
We could also consider adding a boolean (defaulting to false) to the annotation which would control if it affects nested classes as well. WDYT @mkouba?

[snv]

Thanks for having a look at this.

Yes, i have seen no claim that the @ExplicitParamInjection annotation works for nested classes too.
But i think it is sensible to expect that, because this is the usual behavior:

• From a java point of view, an inner class is still inside the same context as the outer class: an inner instance can access fields of outer instances

• In jUnit, the outer configuration is still true for the nested class, for example @ForEach methods will be executed or @DisplayNameGeneration will be honored and extensions loaded will be applied

• Even for weld-junit, the usual behavior matches. Other configurations of the weld extension are still valid inside nested classes, for example producer-methods defined in the outer class or beans included by @AddBeanClasses

[manovotn]

Thanks for having a look at this.

Ah, this issue got completely off my radar, thanks for the reminder!
@snv would you care to submit a PR?

We could also consider adding a boolean (defaulting to false) to the annotation which would control if it affects nested classes as well. WDYT @mkouba?

@mkouba ? :)

[mkouba]

We could also consider adding a boolean (defaulting to false) to the annotation which would control if it affects nested classes as well. WDYT @mkouba?

Another option is to add boolean flag to control the param injection, i.e. @ExplicitParamInjection(true) (default) and @ExplicitParamInjection(false). This way we could honor the annotation from the enclosing class (like in case of WeldInitiator) and support fine-grained control in the nested test classes.

Although the "Explicit" would be a little bit superfluous here and @ParamInjection would be probably a better fit.

[manovotn]

Another option is to add boolean flag to control the param injection, i.e. @ExplicitParamInjection(true) (default) and @ExplicitParamInjection(false). This way we could honor the annotation from the enclosing class (like in case of WeldInitiator) and support fine-grained control in the nested test classes.

Ok, that's very close to what I suggested but has perhaps better default behavior. So +1 and let's do it this way.
@snv if you want to submit a PR with this, let me know. Otherwise I'll add it on my list but no promises in terms of how fast I am going to get to this :)

Although the "Explicit" would be a little bit superfluous here and @ParamInjection would be probably a better fit.

It is what it is and we don't want to break existing apps by changing it.
I am fine with the Explicit part as well because, well, you need to explicitly state qualifiers (even default for instance) to make param injection work. So in my eyes it's pretty close to what it does.

[mkouba]

It is what it is and we don't want to break existing apps by changing it.

👍

[snv]

I am willing to give it a try, but can't make any promises about when and how much time i get to invest there.

[manovotn]

@snv I found some time to look into this and sent #166 which should resolve the issue.
Nested classes will now look into their enclosing class for this annotation unless they explicitly specify it as well.
Classes can also use the newly added parameter to explicitly turn it off.

[snv]

Ah cool, i forked the code and created some tests, but i was still trying to figure out the hirachical-context mechanisms