gh-103186: Suppress and assert expected RuntimeWarnings #103244
Merged
Conversation
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
Caused as a result of frame manipulation where locals are never assigned / initialised
| Most changes to Python require a NEWS entry. Please add it using the blurb_it web app or the blurb command-line tool. |
Contributor Author
| It doesn't look like it's possible to assert both a warning and an error in tandem. #First Attempt if warning is not None and error is not None: with self.assertWarnsRegex(*warning), self.assertRaisesRegex(*error): func(output) #Second Attempt if warning is not None and error is not None: with self.assertWarnsRegex(*warning): with self.assertRaisesRegex(*error): func(output)Regardless of the order of the asserts, only the exception is caught / test failed on regex mismatch - and the warning does not bubble up on regex mismatch (test passes whether a warning is raised or not). I'll try and play around on that avenue further, i.e whether it's possible to catch multiple warnings / errors in succession (I'm assuming it's not) and submit an improvement to the docs with what I learn. The current patch would run an output comparison without running the jump tests if both warning and error arguments are supplied in any new tests (not the case for any existing tests). Updating PR shortly... |
| Most changes to Python require a NEWS entry. Please add it using the blurb_it web app or the blurb command-line tool. |
| Most changes to Python require a NEWS entry. Please add it using the blurb_it web app or the blurb command-line tool. |
terryjreedy requested changes Jul 10, 2023
Member
There was a problem hiding this comment.
On Windows, fresh build (July 10), I get the same runtime warnings as without the patch.
f:\dev\3x>python -m test.test_sys_settrace Running Debug|x64 interpreter... ...f:\dev\3x\Lib\test\test_sys_settrace.py:1895: RuntimeWarning: assigning None to 1 unbound local frame.f_lineno = self.firstLine + self.jumpTo ..f:\dev\3x\Lib\test\test_sys_settrace.py:1895: RuntimeWarning: assigning None to 1 unbound local frame.f_lineno = self.firstLine + self.jumpTo ................................................................................................. f:\dev\3x\Lib\test\test_sys_settrace.py:1818: RuntimeWarning: coroutine method 'aclose' of 'asynciter' was never awaited gc.collect() RuntimeWarning: Enable tracemalloc to get the object allocation traceback ...................................................<traceback object at 0x0000026A38D75EF0> .......................................................................<traceback object at 0x0000026A35F7ABC0> ..................................................................<traceback object at 0x0000026A35F7C690> ...................................................................<traceback object at 0x0000026A38D77F70> ..................................................................<traceback object at 0x0000026A35F7B1B0> ................ ---------------------------------------------------------------------- Ran 439 tests in 6.802s | A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
Contributor Author
| I have made the requested changes; please review again. I'm also getting the additional Will check if this issue persists in the main branch and attempt to patch separately |
| Thanks for making the requested changes! @terryjreedy: please review the changes made to this pull request. |
Contributor Author
It does not, no further action required |
Member
| The 2 'Assigning None' messages, the target of this PR, are gone on a fresh build with this patch added. Good. I am still getting the 3rd warning I and you mentioned above, and the I still need to review the actual code changes and understand them before I could merge. At first glance, it seems that there are more changes than are needed to eliminate the two warnings. Did you add changes for other reasons? |
Contributor Author
Whilst authoring the original change, I noticed that only the initial 2x warnings of the same type / message are printed to console and subsequent warnings are suppressed. This is mentioned in the warnings documentation: https://docs.python.org/3/library/warnings.html |
Member
Do you have an example? |
Contributor Author
We don't currently have a test case that requires both a warning and error to be asserted in tandem, I recall I encountered some issues with basic testing where I attempted to assert both in a single test case. Upon further investigation, it is actually possible to assert both errors and warnings in tandem. On my previous attempts, the execution flow may have been interrupted - this doesn't happen when I raise both explicitly: As such, I'll shortly update this PR once more |
Contributor Author
| @serhiy-storchaka - I've updated this PR code to use contextlib. I've also added an assortment of tandem tests - this helped in identifying that the order of asserting an error's presence, followed by a warning's presence is important in |
serhiy-storchaka approved these changes Sep 6, 2023
Member
serhiy-storchaka left a comment
serhiy-storchaka left a comment There was a problem hiding this comment.
Thank you for your contribution @TabLand .
I removed new tests because they are not relevant here. The purpose of JumpTestCase is to test setting the frame's f_lineno attribute. All other was simply a tool for testing. New tests only tested this tool, exceptions and warnings were raised not by the f_lineno setter.
| There's a new commit after the PR has been approved. @terryjreedy, @serhiy-storchaka: please review the changes made to this pull request. |
serhiy-storchaka added needs backport to 3.12 
Contributor
| Thanks @TabLand for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12. |
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 7, 2023
…_sys_settrace (pythonGH-103244) Caused as a result of frame manipulation where locals are never assigned / initialised. (cherry picked from commit 3e53ac9) Co-authored-by: Ijtaba Hussain <ijtabahussain@live.com>
| GH-109066 is a backport of this pull request to the 3.12 branch. |
|
Yhg1s pushed a commit that referenced this pull request Sep 8, 2023
…t_sys_settrace (GH-103244) (#109066) gh-103186: Suppress and assert expected RuntimeWarnings in test_sys_settrace (GH-103244) Caused as a result of frame manipulation where locals are never assigned / initialised. (cherry picked from commit 3e53ac9) Co-authored-by: Ijtaba Hussain <ijtabahussain@live.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 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.
These warnings are caused due to locals not being assigned, the initialisation lines are skipped in the jump tests:
A similar assertion is present in test_peepholer.py, which was added in the same commit as the unbound locals check in Objects/frameobject.c
c7065ce#diff-bcf92ed219340e9e0c52401909828c2e9a821541660073e24d64dd996beaf842
Hello @brandtbucher - I can see you authored the unbound locals change, any feedback is appreciated