-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
bpo-32374: Fix test_bad_traverse. #7145
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
Conversation
@@ -0,0 +1 @@ | |||
Test added with pull #5140 now does not pass on Python exception raise. |
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 don't think changes to the testsuite need a NEWS entry – and if they did, pull #5140 isn't acceptable shorthand.
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.
It is my understanding that test suite fixes generally do get news entries, since they can affect whether or not python's test suite passes when someone is testing a deployment or vendor package. However, they aren't needed unless the test was introduced in one release and fixed in another (but that does include crossing alpha/beta/release candidate boundaries).
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.
Right, a test for a C-level crash should ignore Python-level exceptions.
with support.SuppressCrashReport(): | ||
m = spec.loader.create_module(spec)""" | ||
# This try block will prevent exceptions from ending the | ||
# process with non-zero status |
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.
At the first read, I failed to understand the purpose of the "except: pass" even with the comment. Moreover, I also missed the comment, so I started to read from the end :-D Maybe move the comment at the bottom, just before "except:"?
I suggest to explain that the test expects a crash as a non-zero exit code. Python exceptions are reported as exit code 0 (success), so the test fails.
nitpick: "# ..." <= only one comment after the dash.
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.
Not really. Mishandling of m_traverse leads to segfault occasionally.
This test is trying to reproduce this specific situation and cause a C-level crash. Problem is, Python exceptions also end the process with non-zero status.
Would this comment be more clear?
"Prevent Python-level exceptions from ending the process with non-zero status (We are testing for a crash in C-code)"
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.
Yes, your proposed comment looks better to me.
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.
LGTM, I just have a question/suggestion on the comment.
I proposed a different approach, explicitly reject exit code 1: PR #7147. |
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.
LGTM.
Thanks @Traceur759 for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7. |
GH-7150 is a backport of this pull request to the 3.7 branch. |
…nGH-7145) (cherry picked from commit 08c5aca) Co-authored-by: Marcel Plch <[email protected]>
…) (GH-7150) (cherry picked from commit 08c5aca) Co-authored-by: Marcel Plch <[email protected]>
The change should also be backported to Python 3.6, no? @encukou |
@vstinner Python 3.6 does not have features which the test is written against. |
I'm talking about this code in the 3.6 branch: It's the commit 1da0479 of bpo-32374. Which feature is missing from 3.6? |
@vstinner Sorry, my bad, I had outdated local branch and didn't see the code there. Backporting is a valid question. |
Thanks @Traceur759 for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6. |
…nGH-7145) (cherry picked from commit 08c5aca) Co-authored-by: Marcel Plch <[email protected]>
GH-7155 is a backport of this pull request to the 3.6 branch. |
I asked @miss-islington to create a PR, I tested it: it works as expected, so I approved the PR #7155. I will be merged as soon as tests pass. |
(cherry picked from commit 08c5aca) Co-authored-by: Marcel Plch <[email protected]>
Previous test assuring that incorrect handling of m_traverse "shouts loudly" on crash
passed also when Python exception was raised.
Fix using try-except block is proposed.
https://bugs.python.org/issue32374