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

avoid custom CreationalContext and Contextual implementations in the TCK #452

Merged
merged 8 commits into from
Sep 21, 2023

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Apr 26, 2023

On a small number of places, the CDI TCK creates its own implementations of CreationalContext and Contextual. These are extremely bare APIs that aren't sufficient for internal needs of CDI implementations or for interoperability. All CDI implementations that I know expose internally a much richer API and generally expect that their CreationalContext and Contextual implementations are used. It is possible to bend the implementations to accept a CreationalContext or Contextual of outside provenance, but I believe there's exactly nothing in the CDI specification that makes this mandatory.

Instead, I believe the CDI TCK should:

  1. avoid using its own CreationalContext and Contextual implementations;
  2. expose SPIs that lets implementations supply their own CreationalContext and Contextual implementations, augmented with inspection capabilities for the tests that need it.

This PR does exactly that.

@Ladicek
Copy link
Contributor Author

Ladicek commented Apr 26, 2023

Since this PR adds a new TCK SPI, this can only be done in the next major/minor CDI TCK version (4.1/5.0 I believe). Hence this is a draft for now.

@manovotn
Copy link
Contributor

I've rebased this onto current master as two of the commits were overlapping with what we did in #463.

Also moving into ready for review state.
@Ladicek Is there anything you'd like to add/change WRT this PR? I already have a Weld side draft for this so I'd be fine to approve and merge this soon.

@manovotn manovotn marked this pull request as ready for review September 21, 2023 08:17
@Ladicek
Copy link
Contributor Author

Ladicek commented Sep 21, 2023

I actually have a few more things locally, to avoid custom Contextuals in addition to custom CreationalContexts. I'll rebase and force-push to this PR.

@Ladicek Ladicek force-pushed the inspectable-creational-context-spi branch from d83a1e3 to 329c666 Compare September 21, 2023 09:33
@Ladicek Ladicek changed the title avoid custom CreationalContext implementations in the TCK avoid custom CreationalContext and Contextual implementations in the TCK Sep 21, 2023
@Ladicek
Copy link
Contributor Author

Ladicek commented Sep 21, 2023

Rebased, added a commit that removes obsolete TCK exclusions, force-pushed, and updated the title and summary of this PR.

@manovotn my implementation of these SPIs for Weld is in https://github.com/Ladicek/weld-core/commits/new-tck-spi, feel free to steal that :-) I also have an implementation of these SPIs for ArC on a branch somewhere, I can dig that up if interested.

@manovotn
Copy link
Contributor

@manovotn my implementation of these SPIs for Weld is in https://github.com/Ladicek/weld-core/commits/new-tck-spi, feel free to steal that :-) I also have an implementation of these SPIs for ArC on a branch somewhere, I can dig that up if interested.

Thanks but I already have my own draft.
Later today I'll add the Contextuals impl to it as well and once I get a pass, I will add a green stamp here.

This commit also splits the `DestroyForSameCreationalContext2Test` test
into a CDI Lite and Full parts.
@Ladicek Ladicek force-pushed the inspectable-creational-context-spi branch from 329c666 to 8520eb8 Compare September 21, 2023 13:00
@manovotn manovotn merged commit 931fb53 into jakartaee:master Sep 21, 2023
4 checks passed
@Ladicek Ladicek deleted the inspectable-creational-context-spi branch September 21, 2023 13:28
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.

2 participants