Skip to content

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

Merged
merged 2 commits into from May 28, 2018
Merged

bpo-32374: Fix test_bad_traverse. #7145

merged 2 commits into from May 28, 2018

Conversation

ghost
Copy link

@ghost ghost commented May 28, 2018

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

@@ -0,0 +1 @@
Test added with pull #5140 now does not pass on Python exception raise.
Copy link
Member

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.

Copy link
Member

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).

Copy link
Member

@encukou encukou left a 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
Copy link
Member

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.

Copy link
Author

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)"

Copy link
Member

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.

Copy link
Member

@vstinner vstinner left a 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.

@vstinner
Copy link
Member

I proposed a different approach, explicitly reject exit code 1: PR #7147.

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM.

@encukou encukou merged commit 08c5aca into python:master May 28, 2018
@miss-islington
Copy link
Contributor

Thanks @Traceur759 for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-7150 is a backport of this pull request to the 3.7 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 28, 2018
encukou pushed a commit that referenced this pull request May 28, 2018
@vstinner
Copy link
Member

The change should also be backported to Python 3.6, no? @encukou

@ghost
Copy link
Author

ghost commented May 28, 2018

@vstinner Python 3.6 does not have features which the test is written against.

@vstinner
Copy link
Member

@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:
https://github.com/python/cpython/blob/3.6/Lib/test/test_importlib/extension/test_loader.py#L270-L285

It's the commit 1da0479 of bpo-32374.

Which feature is missing from 3.6?

@ghost
Copy link
Author

ghost commented May 28, 2018

@vstinner Sorry, my bad, I had outdated local branch and didn't see the code there. Backporting is a valid question.

@miss-islington
Copy link
Contributor

Thanks @Traceur759 for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 28, 2018
@bedevere-bot
Copy link

GH-7155 is a backport of this pull request to the 3.6 branch.

@vstinner
Copy link
Member

@vstinner Sorry, my bad, I had outdated local branch and didn't see the code there. Backporting is a valid question.

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.

miss-islington added a commit that referenced this pull request May 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants