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

Add tests for InvocationContext.getInterceptorBindings() #479

Merged

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Jul 17, 2023

TCK tests for jakartaee/interceptors#99

Draft because there's a temporary profile to enable using Jakarta staging repository. Once Interceptors 2.2.0-RC1 hits Maven Central, I'll remove that and this PR will be ready.

@Ladicek Ladicek requested a review from manovotn July 17, 2023 10:40
@Ladicek Ladicek added this to the CDI 4.1 milestone Jul 17, 2023
@Ladicek Ladicek force-pushed the invocation-context-interceptor-bindings branch 2 times, most recently from c34f1fb to 7e98a47 Compare July 17, 2023 14:18
@Ladicek Ladicek force-pushed the invocation-context-interceptor-bindings branch 2 times, most recently from 8ebed4d to 0a0ea5c Compare July 18, 2023 11:16
@Ladicek
Copy link
Contributor Author

Ladicek commented Jul 18, 2023

This would be ready now, I believe, but let's keep it as a draft until jakartaee/interceptors#103 is merged.

@manovotn manovotn marked this pull request as ready for review September 20, 2023 12:54
@manovotn
Copy link
Contributor

manovotn commented Sep 20, 2023

Moved from draft to ready for review as the linked PR was already approved.
Also main branch of TCKs is now set for 4.1 so we should be able to get this in.

@Ladicek Ladicek force-pushed the invocation-context-interceptor-bindings branch from 5337b72 to 11a3250 Compare September 21, 2023 13:30
@manovotn manovotn merged commit b7a618c into jakartaee:master Oct 13, 2023
2 checks passed
@Ladicek Ladicek deleted the invocation-context-interceptor-bindings branch October 16, 2023 07:14
@Ladicek
Copy link
Contributor Author

Ladicek commented Jan 2, 2024

I just realized we didn't add a test for interceptor bindings added through InterceptionFactory. @manovotn would you have some time to add that? If not, I can try.

@manovotn
Copy link
Contributor

manovotn commented Jan 4, 2024

I just realized we didn't add a test for interceptor bindings added through InterceptionFactory. @manovotn would you have some time to add that? If not, I can try.

Ok, I'll make a note and send a PR

@Azquelt
Copy link
Contributor

Azquelt commented Jan 8, 2024

I think this also doesn't test:

I can make a PR for the second one, and I'd be happy to do the first one as well if we can determine the correct behaviour.

@manovotn
Copy link
Contributor

manovotn commented Jan 8, 2024

Adding an interceptor binding to a method or type using an extension

True, although there is little chance this wouldn't work assuming the interceptor itself is bound correctly (i.e. if that binding was registered, I'd be very surprised if it weren't visible here)

A binding type with additional bindings - not sure if the additional binding should be returned here or not

Good point and those should be returned as well because of the javadoc wording of the method:

* All interceptor binding annotations are returned, including interceptor binding annotations
* that associate interceptors of a different interceptor method type, as well as interceptor
* binding annotations that associate no interceptor.

I can make a PR for the second one, and I'd be happy to do the first one as well if we can determine the correct behaviour.

👍

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

Successfully merging this pull request may close these issues.

3 participants