fix!: ensure that the testing context manager is exited when an exception occurs#2117
Merged
tonyandrewmeyer merged 2 commits intocanonical:mainfrom Oct 19, 2025
Merged
Conversation
james-garner-canonical
approved these changes
Oct 17, 2025
Contributor
james-garner-canonical
left a comment
There was a problem hiding this comment.
LGTM -- quite a mission to figure out that +2/-3 though, wasn't it!
Do you think it's worth adding the 'environment gets cleaned up' tests for non-crashy charms too?
Collaborator
Author
I thought there was already one for this, but can't find it, so yes I've added two more. |
benhoyt
approved these changes
Oct 17, 2025
Collaborator
benhoyt
left a comment
There was a problem hiding this comment.
Thanks guys for the analysis and fix!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Currently, if
testing.Contextis used as a context manager, and an exception occurs during the event, proper cleanup is not done. This is noticeable in two ways in particular:UncaughtCharmError.OPERATOR_DISPATCHis left set.We currently manually call the
__exit__function of one of the context managers when wrapping up. However, if an exception occurs, we never reach that point, and the context manager is left un-exited (you'll also often see errors when exiting Python, because it doesn't like being left in that state). The PR fixes this by also calling__exit__if exiting theContextcontext manager, and it's handling an exception.Many thanks to @james-garner-canonical for helping figure out where things were wrong wrong and a good way to fix this.
Fixes #1926