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

ConditionalObserverTest#testConditionalObserverMethodNotInvokedIfNoActiveContext #447

Closed
manovotn opened this issue Apr 6, 2023 · 3 comments · Fixed by #466
Closed

ConditionalObserverTest#testConditionalObserverMethodNotInvokedIfNoActiveContext #447

manovotn opened this issue Apr 6, 2023 · 3 comments · Fixed by #466
Labels
accepted Issue/challenge is accepted challenge TCK test challenge

Comments

@manovotn
Copy link
Contributor

manovotn commented Apr 6, 2023

Hello,

I recently came across the test in title and I think that it has unfounded expectations.
Namely, the test does the following steps:

  1. Create instance of a bean that holds observer method with reception IF_EXISTS
  2. Manually deactivate context of the bean created in 1. (deactivate, not destroy)
  3. Fire an event to verify that observer of the bean from step 1. isn't notified
  4. Manually re-activate context of the bean from step 1.
  5. Fire a new event and verify that the exact same bean instance from step 1. now got notified

Now, what I find dubious is the step 5. where the test assumes that after reactivating context, the very same bean instance is again present in that context, hence OM with IF_EXIST reception will go through.
Note that activation and deactivation of contexts happens through CDI TCK SPI Contexts. This doesn't have much documentation apart from its javadoc and a mention that it exists in TCK docs.

I couldn't find any part of spec that would support this test in terms of deactivation and subsequent activation implicating that the context state is renewed with the same set of contextual references.

Note 1: The specific part of spec that the test checks (denoted by @SpecAssertion on it) is checked by steps 1 to 4 so the test can be easily altered.
Note 2: Both, Weld and Arc (Quarkus) are able to pass this test, so this is nothing blocking, I am mostly curious whether I am missing something in the spec that would warrant this test.

@manovotn manovotn added the challenge TCK test challenge label Apr 6, 2023
@manovotn
Copy link
Contributor Author

manovotn commented Apr 11, 2023

We discussed this in the meeting today and here are some notable things:

  • The Contexts SPI is pretty underspecified in terms of not saying what should happen to the instances in context between deactivation and activation
  • This might be (to be verified) the only test that relies on reactivation and using the exact same instance
  • Same test would not work with RequestContextController as it explicitly states that between activations, the instances will differ
  • On top of that RequestContextController can only deactivate a context it previously activated
  • The test can be "fixed" by calling tarantula.ping() before attempting to send the event after context reactivation as that will spawn new instance of the bean

@Ladicek
Copy link
Contributor

Ladicek commented Apr 11, 2023

Both Weld and OWB seem to have a very simple "flip the switch" functionality for any given context (likely because they both allow having multiple context objects for the same scope, as the specification expects), so I assume this is what the TCK SPI has always expected. Except it hasn't been written down anywhere. I'd personally be fine with Contexts just specifying this as the required behavior.

@manovotn
Copy link
Contributor Author

manovotn commented May 24, 2023

Given the discussion we had back in the call, I went ahead and sent a PR that simply adds some javadoc to the SPI class making it clear the re-enablement requires restoring context state.

@Ladicek Ladicek added the accepted Issue/challenge is accepted label May 24, 2023
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.

2 participants