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

Initial TCK tests for build compatible extensions #312

Merged
merged 3 commits into from
Dec 2, 2021

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Nov 8, 2021

No description provided.

@Ladicek
Copy link
Contributor Author

Ladicek commented Nov 8, 2021

Marked as draft, because I didn't run these tests with any implementation yet. They are, however, a direct port of tests I developed for my Quarkus prototype, so they should mostly be fine.

I'll work on running these tests with an implementation next.

@Ladicek Ladicek force-pushed the build-compatible-extensions branch from a4e06d7 to e51fffe Compare November 15, 2021 08:51
@Ladicek Ladicek force-pushed the build-compatible-extensions branch from e51fffe to 82af267 Compare November 22, 2021 16:32
@Ladicek
Copy link
Contributor Author

Ladicek commented Nov 22, 2021

Amended the commit with a test for synthetic observers of parameterized types.

The POM now points to the CDI API SNAPSHOT, so this rightfully stays a draft.

@Ladicek Ladicek force-pushed the build-compatible-extensions branch from 82af267 to c61a532 Compare November 23, 2021 15:48
@Ladicek
Copy link
Contributor Author

Ladicek commented Nov 23, 2021

Rebased.

As discussed on the CDI call today, we still don't have a way to run this with an implementation, but work is ongoing to run this both with Weld and Quarkus. Once that happens, this PR will be marked as ready for review and assuming no problems are found, merged. More tests need to be added for sure, but those won't block this PR.

@Ladicek Ladicek force-pushed the build-compatible-extensions branch from c61a532 to 862e391 Compare November 24, 2021 16:51
Copy link
Contributor

@manovotn manovotn left a comment

Choose a reason for hiding this comment

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

I have glued together several snapshots and I am able to test this with Weld.
Most of the tests are failing, so I am going to look into what's the cause and contribute the changes back here.

Meanwhile I dropped a few comments to things I noticed.

@manovotn
Copy link
Contributor

manovotn commented Nov 25, 2021

I have added a commit that fixes ChangeBeanQualifierTest to demonstrate the issue we described in jakartaee/cdi#564
@graemerocher ^ just to make it clear what we meant :)

@manovotn
Copy link
Contributor

Added another commit that fixes all but SyntheticBeanTest where I am not quite sure what the issue is yet.

@manovotn
Copy link
Contributor

Ok, added another commit that fixes the last test. With these changes I can get Weld's implementation to pass it.

@manovotn manovotn marked this pull request as ready for review November 29, 2021 07:42
@manovotn
Copy link
Contributor

Moved from draft to standard PR.
I will also give it green light from myself but maybe @Ladicek would like to review it before merging since I made quite a few changes :)

@Ladicek
Copy link
Contributor Author

Ladicek commented Dec 1, 2021

Hi @manovotn, I've pushed a few more commits with some proposed changes, so that:

  • the tests pass on ArC;
  • multiple combinations of beans.xml present/missing and classes added/not added via @Discovery are tested;
  • this still follows the rules for type discovery as we discussed.

If you can let me know how I can test with Weld, I'd be happy to do that.

@manovotn
Copy link
Contributor

manovotn commented Dec 1, 2021

@Ladicek I am seeing one failing test with Weld. SyntheticObserverOfParameterizedTypeExtension is adding MyService but that class also contains a bean defining annotation and as such is discovered.

The way to execute it with Weld is:

  • Check out master branch
  • Perform a mvn clean install -DskipTests
  • In core/pom.xml, change the value of <cdi.tck-4-0.version> property to 4.0.0-SNAPSHOT
    • This OFC assumes you have CDI TCK built off this PR
  • Run mvn clean verify -f jboss-tck-runner/pom.xml

@Ladicek
Copy link
Contributor Author

Ladicek commented Dec 2, 2021

Added one commit to avoid the type discovery duplicate.

@Ladicek
Copy link
Contributor Author

Ladicek commented Dec 2, 2021

I think this is ready to be merged now, because it passes on ArC and Weld. I'll just restructure the commits a little and squash a bunch of them.

@Ladicek Ladicek force-pushed the build-compatible-extensions branch from 9177823 to 48015e8 Compare December 2, 2021 15:22
@Ladicek
Copy link
Contributor Author

Ladicek commented Dec 2, 2021

Done.

@manovotn manovotn merged commit 50a3e3d into jakartaee:master Dec 2, 2021
@Ladicek Ladicek deleted the build-compatible-extensions branch December 3, 2021 07:54
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.

4 participants