Skip to content

bpo-37421: Fix test_distutils.test_build_ext() #14564

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 1 commit into from
Jul 3, 2019
Merged

bpo-37421: Fix test_distutils.test_build_ext() #14564

merged 1 commit into from
Jul 3, 2019

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Jul 2, 2019

test_distutils.test_build_ext() is now able to remove the temporary
directory on Windows: don't import the newly built C extension ("xx")
in the current process, but test it in a separated process.

https://bugs.python.org/issue37421

test_distutils.test_build_ext() is now able to remove the temporary
directory on Windows: don't import the newly built C extension ("xx")
in the current process, but test it in a separated process.
Copy link
Contributor

@aeros aeros left a comment

Choose a reason for hiding this comment

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

@vstinner Thanks for changing the variable d to tmpdir on line 67 of support.py. In my opinion, non-descriptive variables can be the bane of readability.

Is there a particular reason or advantage in this case for wrapping the tests within a whitespace-stripped string instead of writing the Tests class within another file? I have no issues with it, I just haven't seen it done before.

@vstinner
Copy link
Member Author

vstinner commented Jul 3, 2019

@vstinner Thanks for changing the variable d to tmpdir on line 67 of support.py. In my opinion, non-descriptive variables can be the bane of readability.

You're welcome.

Is there a particular reason or advantage in this case for wrapping the tests within a whitespace-stripped string instead of writing the Tests class within another file? I have no issues with it, I just haven't seen it done before.

It's more convenient to put everything at the same place. It's a very common pattern if you look at the Python test suite.

@vstinner vstinner merged commit 74c9dd5 into python:master Jul 3, 2019
@vstinner vstinner deleted the distutils_cleanup branch July 3, 2019 09:12
@miss-islington
Copy link
Contributor

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 3, 2019
test_distutils.test_build_ext() is now able to remove the temporary
directory on Windows: don't import the newly built C extension ("xx")
in the current process, but test it in a separated process.
(cherry picked from commit 74c9dd5)

Co-authored-by: Victor Stinner <[email protected]>
@bedevere-bot
Copy link

GH-14571 is a backport of this pull request to the 3.8 branch.

miss-islington added a commit that referenced this pull request Jul 3, 2019
test_distutils.test_build_ext() is now able to remove the temporary
directory on Windows: don't import the newly built C extension ("xx")
in the current process, but test it in a separated process.
(cherry picked from commit 74c9dd5)

Co-authored-by: Victor Stinner <[email protected]>
@aeros
Copy link
Contributor

aeros commented Jul 4, 2019

@vstinner Thanks for the explanation. I'm a newer contributor, so I'm not overly familiar with the common patterns within the Python test suite. However, I definitely am a fan of how compact it is, I'll definitely keep the format in mind. After finishing up reorganization of test_importlib, Brett had suggested that it'd be helpful to work on modernizing the tests over there. If possible, I'll use this format as a template.

lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2019
test_distutils.test_build_ext() is now able to remove the temporary
directory on Windows: don't import the newly built C extension ("xx")
in the current process, but test it in a separated process.
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
test_distutils.test_build_ext() is now able to remove the temporary
directory on Windows: don't import the newly built C extension ("xx")
in the current process, but test it in a separated process.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants