-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Revert test serializer changes to avoid regressing existing use cases #40880
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥲
This comment has been minimized.
This comment has been minimized.
😭 |
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. |
0c412f7
to
ef9c6c3
Compare
I got confused by my towering stack of disabled tests and test-related issues, and enabled the wrong test. Hopefully fixed now. :) |
This comment has been minimized.
This comment has been minimized.
ef9c6c3
to
cc4cfe0
Compare
This comment has been minimized.
This comment has been minimized.
cc4cfe0
to
fc7357b
Compare
fc7357b
to
6fd669b
Compare
There was a problem hiding this 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.
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. 🥳 |
We now have JUnit |
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