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

Revert test serializer changes to avoid regressing existing use cases #40880

Merged
merged 3 commits into from
May 30, 2024

Conversation

holly-cummins
Copy link
Contributor

@holly-cummins holly-cummins commented May 29, 2024

This reverts #40601, to fix quarkiverse/quarkiverse#94.
As part of the same changeset, I’ve also disabled a test that #40601 caused to pass (#40749), and enabled a test that #40601 makes fail (#40773).

Why is this a change we'd want, if it makes stuff fail? IMO, the stuff it makes pass is more important than the stuff it makes fail. The test which this PR now causes to fail has never passed, but the test which it gets to pass is reasonably widely used in the field. We don't want to regress it. How bad is the regression? Anything involving a Pact provider won’t work without this change. We should, IMO, favour ‘avoiding regressions’ over ‘enabling new functionality’. If 3.12 ships with a regression, that’s embarrassing, and if it has a regression that we actually knew about and decided we didn't care about, that’s something that will affect user trust in us.

Does this mean that #40601 isn’t a good change? No! It's an awesome change. This is a temporary reversion, just until junit-team/junit5#3820 makes it into a JUnit release. Think of it as a delay to function we definitely want, not a reversion. :)

This PR should be reverted as soon as we have a release of JUnit with junit-team/junit5#3820 in it. I’ve split the PR into two parts; we only want to revert 8e7f808, because hopefully the test enabled by other commit will continue passing. Once/if this merges, I'll open another PR with the reversion of the reversion (!), to make sure we don't forget.

There’s a fuller discussion of the arguments for and against this change on #40751 (which is an alternate solution to get these use cases passing).

cc @dmlloyd @geoand @gsmet @edeandrea

@quarkus-bot quarkus-bot bot added area/dependencies Pull requests that update a dependency file area/maven area/testing labels May 29, 2024
Copy link
Member

@dmlloyd dmlloyd left a comment

Choose a reason for hiding this comment

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

🥲

@quarkus-bot

This comment has been minimized.

@geoand
Copy link
Contributor

geoand commented May 29, 2024

😭

@edeandrea
Copy link
Contributor

As much as I hate reverting stuff, I agree with the mindset and direction here. Better to not knowingly introduce a regression to something people use in favor of enabling something which has never been possible, especially knowing that there is a fix in the pipeline that will eventually make everyone happy.

@holly-cummins holly-cummins force-pushed the revert-test-serializer-changes branch 2 times, most recently from 0c412f7 to ef9c6c3 Compare May 29, 2024 14:12
@holly-cummins
Copy link
Contributor Author

I got confused by my towering stack of disabled tests and test-related issues, and enabled the wrong test. Hopefully fixed now. :)

@quarkus-bot

This comment has been minimized.

@holly-cummins holly-cummins force-pushed the revert-test-serializer-changes branch from ef9c6c3 to cc4cfe0 Compare May 29, 2024 14:59
@quarkus-bot

This comment has been minimized.

@holly-cummins holly-cummins force-pushed the revert-test-serializer-changes branch from fc7357b to 6fd669b Compare May 30, 2024 09:31
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

I agree.

Let's monitor JUnit releases so that we can push it back as soon as we have a fix for the issue in JUnit.

@gsmet gsmet merged commit 50e732f into quarkusio:main May 30, 2024
51 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.12 - main milestone May 30, 2024
@holly-cummins holly-cummins deleted the revert-test-serializer-changes branch May 30, 2024 17:03
@holly-cummins
Copy link
Contributor Author

I've raised #40906 so we don't lose track of getting #40601 back in. We obviously can't merge it yet for all sorts of reasons, but it's there, with the right code changes for serialization.

@edeandrea, you can re-enable the superheroes tests now. 🥳

@edeandrea
Copy link
Contributor

@geoand
Copy link
Contributor

geoand commented Jun 28, 2024

We now have JUnit 5.10.3 in main which includes David's fix, so I pressume we can bring #40601 back

@holly-cummins
Copy link
Contributor Author

holly-cummins commented Jun 28, 2024

We now have JUnit 5.10.3 in main which includes David's fix, so I pressume we can bring #40601 back

(Almost) ready to go in #40906. I did it as soon as #40601 came out so we wouldn't lose track of it. :) Sadly, we have a failure in the 'fails with #40601' test that needs investigation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI] - Pact + Quarkus main
5 participants