Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Intercept unit test method #144

Open
graben opened this issue Sep 2, 2022 · 14 comments
Open

Intercept unit test method #144

graben opened this issue Sep 2, 2022 · 14 comments
Milestone

Comments

@graben
Copy link

graben commented Sep 2, 2022

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

@mkouba
Copy link
Member

mkouba commented Sep 2, 2022

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

@manovotn
Copy link
Collaborator

manovotn commented Sep 2, 2022

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
Copy link
Collaborator

manovotn commented Sep 2, 2022

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?

@manovotn manovotn closed this as completed Sep 2, 2022
@mkouba
Copy link
Member

mkouba commented Sep 2, 2022

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

@graben
Copy link
Author

graben commented Sep 2, 2022

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
Copy link
Collaborator

manovotn commented Sep 2, 2022

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

But note that this brings some limitations:

  • There can only be one extension doing this so it might clash when used with other extensions
  • Weld extension must only be declared on class level
  • I think there is also an issue that nested tests cannot be beans due to CDI spec limitations
    • And possibly other similar limitations, basically the test class need to pass for a full fledged CDI bean at that point
  • ATM we are booting up Weld based on user preferences which are either annotation based (which is fine) or written in WeldInitiator which is a field - this would no longer be possible
  • Don't know if there is anything else...

@manovotn
Copy link
Collaborator

manovotn commented Sep 2, 2022

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
Copy link
Collaborator

manovotn commented Sep 2, 2022

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

@manovotn
Copy link
Collaborator

manovotn commented Sep 2, 2022

@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
Copy link
Author

graben commented Sep 2, 2022

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

@manovotn
Copy link
Collaborator

manovotn commented Sep 2, 2022

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

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

@manovotn manovotn reopened this Sep 2, 2022
@Vampire
Copy link
Contributor

Vampire commented Sep 2, 2022

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
Copy link
Collaborator

manovotn commented Sep 2, 2022

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
Copy link
Collaborator

manovotn commented Sep 2, 2022

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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants