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

DependentContextTest#testDependentScopedInterceptorsAreDependentObjectsOfBean() does not test the spec assertion #454

Closed
mkouba opened this issue Apr 27, 2023 · 7 comments · Fixed by #462
Assignees
Labels
accepted Issue/challenge is accepted challenge TCK test challenge

Comments

@mkouba
Copy link
Contributor

mkouba commented Apr 27, 2023

The org.jboss.cdi.tck.tests.context.dependent.DependentContextTest#testDependentScopedInterceptorsAreDependentObjectsOfBean() test method does not test the linked spec assertion:

Instances of interceptors or decorators with scope @Dependent are dependent objects of the bean instance they decorate.

It merely asserts that if CreationalContext#release() is called then the TransactionalInterceptor#destroy() callback is invoked because the AccountTransaction bean is destroyed. And even this assert is questionable because the test should rather call Bean#destroy() instead; CreationalContext#release() should destroy the dependent objects and not the bean itself.

In fact, this assertion should be testable=false because there's no reasonable way to test it (an interceptor may not declare its own @PreDestroy callback).

I'd recommend to mark the assertion as testable=false and remove the test method completely.

@manovotn manovotn added the challenge TCK test challenge label Apr 27, 2023
@Azquelt
Copy link
Contributor

Azquelt commented May 10, 2023

Could you test this indirectly by injecting a @Dependent bean into the interceptor and ensuring that it gets destroyed (which should only happen if the interceptor itself is destroyed)?

@Ladicek
Copy link
Contributor

Ladicek commented May 11, 2023

That is a good idea! I'll see if I can rewrite the test like that.

@Ladicek
Copy link
Contributor

Ladicek commented May 11, 2023

Seems to work well -- submitted #462. Thanks @Azquelt!

@manovotn
Copy link
Contributor

Very good idea indeed, that should do the trick!

@manovotn manovotn added the accepted Issue/challenge is accepted label May 11, 2023
@manovotn
Copy link
Contributor

Added the accepted label as we seem to agree that the current state isn't OK and we now have a way to test it properly

@manovotn
Copy link
Contributor

As discussed in the meeting today, the solution to this challenge needs to be a test exclusion.
The challenge is correct and so is the fix. However, previous version of the TCK only passed with Weld due to a Weld bug (see https://issues.redhat.com/browse/WELD-2743).
Therefore, in order for Weld releases to keep passing latest TCKs, the correct course of action is excluding this test for 4.0.x and re-enabling them for 4.1. Otherwise, we'll repeat the situation where older Weld version won't pass latest TCKs.

I'll send a PR for this.

@manovotn manovotn reopened this May 23, 2023
@manovotn manovotn self-assigned this May 23, 2023
@manovotn
Copy link
Contributor

As discussed in the meeting today, the solution to this challenge needs to be a test exclusion. The challenge is correct and so is the fix. However, previous version of the TCK only passed with Weld due to a Weld bug (see https://issues.redhat.com/browse/WELD-2743). Therefore, in order for Weld releases to keep passing latest TCKs, the correct course of action is excluding this test for 4.0.x and re-enabling them for 4.1. Otherwise, we'll repeat the situation where older Weld version won't pass latest TCKs.

I'll send a PR for this.

@Ladicek @Emily-Jiang it seems that I was mistaken and that we got a little confused in the mtg discussion yesterday :)
Weld actually passes this test (after changes made in #462) even on older versions - I tried with 5.0.0.Final.
What wouldn't work is Weld 5.1.1 (not yet released) with older TCK version, such as 4.0.9. However, that doesn't really matter as the requirement is for all impl versions to pass the most recent TCKs.
Therefore, no action is needed here - sorry for the hassle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Issue/challenge is accepted challenge TCK test challenge
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants