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

Change AfterTypeDiscoveryTest to avoid assumptions about how many tot… #401

Merged
merged 1 commit into from
Jul 27, 2022

Conversation

manovotn
Copy link
Contributor

…al components are in test deployment.

Fixes #400

@manovotn
Copy link
Contributor Author

@erdlet This PR should fix your problems, do you think you could checkout this branch, build it (just mvn clean install will do) and test it with GF?

@erdlet
Copy link

erdlet commented Jul 27, 2022

Thanks a lot for this fix. I'll test it until this evening against GF and provide feedback to you.

@manovotn manovotn marked this pull request as ready for review July 27, 2022 12:23
@manovotn manovotn requested a review from Ladicek July 27, 2022 12:23
@manovotn manovotn added challenge TCK test challenge accepted Issue/challenge is accepted labels Jul 27, 2022
@manovotn
Copy link
Contributor Author

I have marked the PR and the issue as challenge and accepted for I believe in this case the test makes assumptions that it shouldn't. Nonetheless, if we verify that this fixes the problem for you, we can make a quick TCK release if we chose to :)

@erdlet
Copy link

erdlet commented Jul 27, 2022

@manovotn I just ran the TCK built from your branch and ran it against GF 7 with MVC 2.1.0.RC1 and Krazo 3.0.0.RC1.

It seems this test is broken too: AfterTypeDiscoveryMassOperationsTest>Arquillian.run:138->testInitialAlternatives:77 expected [3] but found [11].
I guess It takes the same assumptions as the test you already fixed. Can you please have a look at it too? The fixed test works like a charm :)

@manovotn
Copy link
Contributor Author

@erdlet good find!
Both tests should now be fixed as part of this PR :)

@erdlet
Copy link

erdlet commented Jul 27, 2022

@manovotn thanks for the quick fix! I'll build and run the TCK again so I should've feedback in a few minutes if everything is fine now :)

@erdlet
Copy link

erdlet commented Jul 27, 2022

@manovotn great, it works 👍🏻 Thanks a lot for your help!

Nonetheless, if we verify that this fixes the problem for you, we can make a quick TCK release if we chose to

Would it be possible to release the TCK within the next days so I can create a PR to re-integrate MVC into GF? :)

@manovotn
Copy link
Contributor Author

@erdlet glad to hear that.
And yes, I'll do the release tomorrow and will let you know once its out.

@manovotn manovotn merged commit b4ce6fe into jakartaee:master Jul 27, 2022
@manovotn manovotn deleted the issue400 branch July 27, 2022 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Issue/challenge is accepted challenge TCK test challenge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Questions about why CDI TCK fails when Jakarta MVC / Eclipse Krazo are bundled in Glassfish 7
3 participants