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

Changes to pass the DI TCK #550

Merged
merged 3 commits into from
Apr 6, 2021
Merged

Conversation

Cousjava
Copy link
Member

@Cousjava Cousjava commented Mar 24, 2021

This changes HK2 injection order to match that required by TCK.
It also changes behaviour when encountering static injection to ignore it rather than throwing an error as that prevents the TCK from running. With this change HK2 now passes the DI TCK, a runner for which is now included in the test modules.

Signed-off-by: Jonathan Coustick <[email protected]>
@smillidge smillidge requested a review from jansupol March 31, 2021 10:26
@smillidge
Copy link
Contributor

smillidge commented Mar 31, 2021

@jansupol @m0mus would this affect Jersey? We are trying to get HK2 as a Compatible Implementation of DI

Copy link
Contributor

@Pandrex247 Pandrex247 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a field of expertise of mine, but the tests pass and the TCK passes.
I spoke to you in Teams about the changes made to NegativeMethodTest#testStaticMethod and NegativeFieldTest#testStaticField. For transparency: In short, those tests now check that nothing gets injected rather than checking an exception gets thrown when attempting to inject

Copy link
Contributor

@arjantijms arjantijms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At a glance this looks very good! Thanks Jonathan!

Just a question, did you check whether Payara and/or GlassFish still passes all tests (e.g. quicklook, samples, tck, etc) with this?

@smillidge smillidge requested a review from m0mus March 31, 2021 12:02
@jansupol
Copy link
Contributor

@smillidge From the Jersey perspective, I'd say the change is ok, since what it threw the exception before, now works. But we would need to have the HK2 (perhaps staging) at hand to run the tests to be sure.

@jansupol
Copy link
Contributor

I have built HK2 with this PR and internal Jersey tests (not TCK) passed with this HK2 change.

@arjantijms
Copy link
Contributor

Thanks @jansupol for the extra testing!

@arjantijms arjantijms merged commit eb8680c into eclipse-ee4j:master Apr 6, 2021
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.

5 participants