weld / weld-testing

Set of test framework extensions (JUnit 4, JUnit 5, Spock) to enhance the testing of CDI components via Weld. Supports Weld 5.
http://weld.cdi-spec.org/
Apache License 2.0
102 stars 30 forks source link

Intercept unit test method #144

Open graben opened 2 years ago

graben commented 2 years ago

Is it possible to do intercepting for test method itself? Handle test instance as a bean.

mkouba commented 2 years ago

I don't think that it's possible in the current implementation but @manovotn would know for sure ;-).

manovotn commented 2 years ago

Hi @graben

It is not possible. If memory serves, this wasn't doable due to how JUnit handles test instances. Since our implementation is a JUnit extension, Junit provides already instantiated test classes to us. And I don't think there was a way to do it differently. For interception/decoration to work, Weld would need to create those classes and hand them over to JUnit instead.

manovotn commented 2 years ago

I am not sure why do you need this but note that in order to "intercept" (in a non-CDI manner) test methods, you can do that fairly simply by writing a custom JUnit 5 extension. Maybe that would help?

mkouba commented 2 years ago

Hm, it could be possible in JUnit 5.7+ with org.junit.jupiter.api.extension.TestInstanceFactory.

graben commented 2 years ago

Yes, that might be the trick to switch weld creation to TestInstanceFactory lifecycle method. Maybe look at https://github.com/cschabl/cdi-unit-junit5

manovotn commented 2 years ago

That's interesting, didn't know about it.

But note that this brings some limitations:

manovotn commented 2 years ago

Yes, that might be the trick to switch weld creation to TestInstanceFactory lifecycle method. Maybe look at https://github.com/cschabl/cdi-unit-junit5

It seems they are doing full discovery which is very inefficient if there are more tests and possibly interferes with configs between tests. We OTOH try to let users create minimal archive but for that, we need to know what classes are to be added to the CDI container...

manovotn commented 2 years ago

Also, didn't we have a similar conversation somewhere with @Vampire?

manovotn commented 2 years ago

@graben what was the original use case why you needed to intercept instances?

The more I think about it, the more it seems to me that going for TestInstanceFactory makes us lose more than we gain by it. I can OFC be convinced otherwise but I am not seeing strong argument for going in that direction and dropping parts of functionalities that we have now.

graben commented 2 years ago

@manovotn: I try to test my own transational cdi extension that's why my test methods uses @Transactional

manovotn commented 2 years ago

Ok, let's keep this issue open for more discussion.

@mkouba what's your take on whether it is worth it?

Vampire commented 2 years ago

I cannot remember such a discussion, but that could also be caused by my holey memory.

But another thing I remember is, that somehow (at least with the JUnit Jupiter and then probably also the Spock variant) the test instance provided by the test framework was put in place of an own instance generated by Weld. Don't remember the gory details right now.

manovotn commented 2 years ago

I cannot remember such a discussion, but that could also be caused by my holey memory.

Ok, I might be misremembering it then, sorry for the noise.

But another thing I remember is, that somehow (at least with the JUnit Jupiter and then probably also the Spock variant) the test instance provided by the test framework was put in place of an own instance generated by Weld. Don't remember the gory details right now.

Yes, this is basically what we're talking about here. We'd need to be able to bootstrap Weld and use it to provide test instance way sooner for it to be interceptable.

manovotn commented 2 years ago

Theoretically, this could be done for WeldJunit5AutoExtension without much friction as Weld container setup is based purely on annotations readable from Class objects. Plus this is deliberately incompatible with declaring WeldInitiator.

However, that creates discrepancy between how WeldJunit5AutoExtension and how WeldJunit5Extension works. The latter would not be able to support this, at least not for any WeldInitiator that isn't declared as static. From there we can either say that non-static are not supported - this would be a breaking change but also a clean solution. Alternatively, we can fallback to not supporting interception and instantiating test class without CDI - in this case we'll still have to implement TestInstanceFactory despite not using it (meaning we still clash with other extensions using it) and we'll spend some time introspecting the test hierarchy via reflection that is going to be pretty much wasted.

However, the above still presumes that the test class can become managed bean as per specification. Mainly the no-arq constructor and not being an inner class presenting a potential blocker. To be able to deal with those, we'd still need the fallback option, as outlined above.

I'll get back to this next week since I need to refresh my memory on how exactly do we integrate with JUnit. From there, I'll see if I can spot some reasonable way to cobble this together...