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

Fix CustomCcmRule to drop CURRENT flag no matter what #1961

Open
wants to merge 1 commit into
base: 4.x
Choose a base branch
from

Conversation

dkropachev
Copy link

It is follow up for #1720

If super.after() throws an Exception CURRENT flag is never dropped which leads next tests with CustomCcmMRule to fail with IllegalStateException("Attempting to use a Ccm rule while another is in use. This is disallowed")

If super.after() throws an Exception `CURRENT` flag is never dropped
which leads next tests to fail with IllegalStateException("Attempting to use a Ccm rule while another is in use.  This is disallowed")
@dkropachev
Copy link
Author

@lukasz-antoniak , @tolbertam, could you please take a look at it

@tolbertam tolbertam self-requested a review September 14, 2024 11:23
Copy link
Contributor

@tolbertam tolbertam left a comment

Choose a reason for hiding this comment

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

Thanks for the follow up on this @dkropachev, agreed on this change.

ccm remove failing (in super.after()) may indicate some odd/fatal state, but I agree that not resetting CURRENT assuredly leaves things in a state where further tests can't possibly run and it makes sense to make a best effort to allow tests proceed 👍

@absurdfarce absurdfarce self-requested a review September 18, 2024 21:17
@absurdfarce
Copy link
Contributor

Agree that the current behaviour is incorrect; an exception in super.after() shouldn't lead to subsequent failures. I've kicked off a Jenkins run to make sure there isn't any unexpected regression here (I'll be very surprised if there is) so once that's complete with issue this gets a +1 from me.

@absurdfarce
Copy link
Contributor

Jenkins run looks good: one test failure but it's from a test that's known to fail intermittently.

@tolbertam
Copy link
Contributor

tolbertam commented Sep 19, 2024

Thanks for the review and the testing @absurdfarce!

@dkropachev, if you could, can you amend your commit to include this in the last line and force push? Otherwise if you can give me access to the branch I can amend it as well:

Patch by Dmitry Kropachev; reviewed by Andy Tolbert and Bret McGuire for JAVA-3117

(Reusing JAVA-3117 as this was a follow on). After that I'll merge it, thanks!

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.

3 participants