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.

Sign up for GitHub

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

Jump to bottom

avoid custom CreationalContext and Contextual implementations in the TCK #452

Merged
merged 8 commits into from
Sep 21, 2023
Merged

avoid custom CreationalContext and Contextual implementations in the TCK #452

merged 8 commits into from
Sep 21, 2023

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Apr 26, 2023 •
edited
Loading

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.

@Ladicek Ladicek requested a review from manovotn April 26, 2023 11:54
@Ladicek Ladicek force-pushed the inspectable-creational-context-spi branch from 0322c74 to c7789ab Compare April 26, 2023 12:02
@manovotn manovotn mentioned this pull request Apr 26, 2023
Closed
@Ladicek Ladicek mentioned this pull request May 18, 2023
Merged
@Ladicek Ladicek added this to the CDI 4.1 milestone May 23, 2023
@manovotn manovotn force-pushed the inspectable-creational-context-spi branch from c7789ab to d83a1e3 Compare September 21, 2023 08:15
@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.

@manovotn manovotn mentioned this pull request Sep 21, 2023
Merged
@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 •
edited
Loading

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.

@Ladicek
add the Contextuals SPI
9e4ceb5
@Ladicek
replace usages of DummyContextual with the Contextuals SPI
f1a204e 
This commit also splits the `DestroyForSameCreationalContext2Test` test
into a CDI Lite and Full parts.
@Ladicek
remove unused DummyContextual
8520eb8
@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
2 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
Reviewers

@manovotn manovotn manovotn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
CDI 4.1
Development

Successfully merging this pull request may close these issues.
2 participants
@Ladicek @manovotn