[proxy] web.archive.org — ← back | site home | direct (HTTPS) ↗ | proxy home | ◑ dark◐ light
/ cpython Public
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

Jump to bottom

bpo-32374: Fix test_bad_traverse. #7145

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

Copy link

ghost commented May 28, 2018 •
edited by bedevere-bot

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

ghost force-pushed the bad_traverse_patch branch from e12474e to f72ed52 Compare May 28, 2018
encukou reviewed May 28, 2018
View changes
Misc/NEWS.d/next/Tests/2018-05-28-10-58-39.bpo-32374.NErc2R.rst Outdated
@@ -0,0 +1 @@
Test added with pull #5140 now does not pass on Python exception raise.
Copy link
Member

encukou May 28, 2018

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

bitdancer May 28, 2018

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

ghost force-pushed the bad_traverse_patch branch from f72ed52 to 1cfb323 Compare May 28, 2018
encukou approved these changes May 28, 2018
View changes
Copy link
Member

encukou left a comment

Right, a test for a C-level crash should ignore Python-level exceptions.

vstinner reviewed May 28, 2018
View changes
Lib/test/test_importlib/extension/test_loader.py Outdated
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

vstinner May 28, 2018

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

ghost May 28, 2018

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

vstinner May 28, 2018

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.

vstinner approved these changes May 28, 2018
View changes
Copy link
Member

vstinner left a comment

LGTM, I just have a question/suggestion on the comment.

Copy link
Member

vstinner commented May 28, 2018

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

vstinner reviewed May 28, 2018
View changes
Copy link
Member

vstinner left a comment

LGTM.

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

miss-islington commented May 28, 2018

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

Copy link

bedevere-bot commented May 28, 2018

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
bpo-32374: Ignore Python-level exceptions in test_bad_traverse (pytho…
f0f52c8 
…nGH-7145)

(cherry picked from commit 08c5aca)

Co-authored-by: Marcel Plch <gmarcel.plch@gmail.com>
encukou pushed a commit that referenced this pull request May 28, 2018
bpo-32374: Ignore Python-level exceptions in test_bad_traverse (GH-7145…
983c1ba 
…) (GH-7150)

(cherry picked from commit 08c5aca)

Co-authored-by: Marcel Plch <gmarcel.plch@gmail.com>
Copy link
Member

vstinner commented May 28, 2018

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

Copy link
Author

ghost commented May 28, 2018

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

Copy link
Member

vstinner commented May 28, 2018

@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?

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.

Copy link
Contributor

miss-islington commented May 28, 2018

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
bpo-32374: Ignore Python-level exceptions in test_bad_traverse (pytho…
8378791 
…nGH-7145)

(cherry picked from commit 08c5aca)

Co-authored-by: Marcel Plch <gmarcel.plch@gmail.com>
Copy link

bedevere-bot commented May 28, 2018

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

Copy link
Member

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

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
bpo-32374: Ignore Python-level exceptions in test_bad_traverse (GH-7145)
6ec325d 
(cherry picked from commit 08c5aca)

Co-authored-by: Marcel Plch <gmarcel.plch@gmail.com>
yahya-abou-imran pushed a commit to yahya-abou-imran/cpython that referenced this pull request Nov 2, 2018
bpo-32374: Ignore Python-level exceptions in test_bad_traverse (pytho…
d5f8ff1 
…nGH-7145)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants