bpo-35978: Correctly skips venv tests in venvs by zooba · Pull Request #12220 · python/cpython

zooba · 2019-03-07T17:27:43Z

As well as changing the skip decorator, I also slightly reordered the handling of running venv out of a build directory on Windows to minimize the work in the normal (non-build dir) case, and to also handle the venv-from-venv-from-build-directory case (where sysconfig doesn't correctly detect it).

https://bugs.python.org/issue35978

vstinner · 2019-03-08T09:11:56Z

I prefer the current skip message:

test_with_pip (test.test_venv.EnsurePipTest) ... skipped 'Test not appropriate in a venv'

The new one is more surprising:

test_with_pip (test.test_venv.EnsurePipTest) ... skipped 'Test requires venv.create'

I don't get it: venv.create() doesn't exist? Example with test_posix:

test_pwritev_flags (test.test_posix.PosixTester) ... skipped 'test needs os.RWF_SYNC'

Usually "need xxx" means that a function is not available on a platform.

vstinner · 2019-03-08T09:12:11Z

I confirm that this PR fix https://bugs.python.org/issue35978 on Linux.

vstinner@apu$ ./python -m venv venv

vstinner@apu$ ./python -m test test_venv -v
== CPython 3.8.0a2+ (heads/master:dc078947a5, Mar 7 2019, 13:03:13) [GCC 8.3.1 20190223 (Red Hat 8.3.1-2)]
== Linux-4.20.13-200.fc29.x86_64-x86_64-with-glibc2.28 little-endian
== cwd: /home/vstinner/prog/python/master/build/test_python_29002
== CPU count: 8
== encodings: locale=UTF-8, FS=utf-8
Run tests sequentially
0:00:00 load avg: 0.57 [1/1] test_venv
test_defaults (test.test_venv.BasicTest) ... ok
test_executable (test.test_venv.BasicTest) ... ok
test_executable_symlinks (test.test_venv.BasicTest) ... ok
test_isolation (test.test_venv.BasicTest) ... ok
test_multiprocessing (test.test_venv.BasicTest) ... ok
test_overwrite_existing (test.test_venv.BasicTest) ... ok
test_prefixes (test.test_venv.BasicTest) ... ok
test_prompt (test.test_venv.BasicTest) ... ok
test_symlinking (test.test_venv.BasicTest) ... ok
test_unicode_in_batch_file (test.test_venv.BasicTest) ... skipped 'only relevant on Windows'
test_unoverwritable_fails (test.test_venv.BasicTest) ... ok
test_upgrade (test.test_venv.BasicTest) ... ok
test_devnull (test.test_venv.EnsurePipTest) ... ok
test_explicit_no_pip (test.test_venv.EnsurePipTest) ... ok
test_no_pip_by_default (test.test_venv.EnsurePipTest) ... ok
test_with_pip (test.test_venv.EnsurePipTest) ... ok

----------------------------------------------------------------------

Ran 16 tests in 19.850s

OK (skipped=1)

== Tests result: SUCCESS ==

1 test OK.

Total duration: 19 sec 968 ms
Tests result: SUCCESS
vstinner@apu$ ./venv/bin/python -m test test_venv -v
== CPython 3.8.0a2+ (heads/master:dc078947a5, Mar 7 2019, 13:03:13) [GCC 8.3.1 20190223 (Red Hat 8.3.1-2)]
== Linux-4.20.13-200.fc29.x86_64-x86_64-with-glibc2.28 little-endian
== cwd: /home/vstinner/prog/python/master/build/test_python_29042
== CPU count: 8
== encodings: locale=UTF-8, FS=utf-8
Run tests sequentially
0:00:00 load avg: 0.64 [1/1] test_venv
test_defaults (test.test_venv.BasicTest) ... ok
test_executable (test.test_venv.BasicTest) ... skipped 'Test requires venv.create'
test_executable_symlinks (test.test_venv.BasicTest) ... ok
test_isolation (test.test_venv.BasicTest) ... ok
test_multiprocessing (test.test_venv.BasicTest) ... skipped 'Test requires venv.create'
test_overwrite_existing (test.test_venv.BasicTest) ... ok
test_prefixes (test.test_venv.BasicTest) ... skipped 'Test requires venv.create'
test_prompt (test.test_venv.BasicTest) ... ok
test_symlinking (test.test_venv.BasicTest) ... ok
test_unicode_in_batch_file (test.test_venv.BasicTest) ... skipped 'only relevant on Windows'
test_unoverwritable_fails (test.test_venv.BasicTest) ... ok
test_upgrade (test.test_venv.BasicTest) ... ok
test_devnull (test.test_venv.EnsurePipTest) ... skipped 'Test requires venv.create'
test_explicit_no_pip (test.test_venv.EnsurePipTest) ... skipped 'Test requires venv.create'
test_no_pip_by_default (test.test_venv.EnsurePipTest) ... skipped 'Test requires venv.create'
test_with_pip (test.test_venv.EnsurePipTest) ... skipped 'Test requires venv.create'

----------------------------------------------------------------------

Ran 16 tests in 0.119s

OK (skipped=8)

== Tests result: SUCCESS ==

1 test OK.

Total duration: 236 ms
Tests result: SUCCESS

vstinner · 2019-03-08T09:13:52Z

Lib/test/test_venv.py

+requireVenvCreate = unittest.skipUnless(
+    sys.platform.startswith('win')
+    or sys.prefix == sys.base_prefix,
+    'Test requires venv.create')


I don't understand why you changed the decorator name and the skip message. I don't understand the new skip message (see my other comment on the PR). Why not just add sys.platform.startswith('win') in skipInVenv?

vstinner · 2019-03-08T09:14:42Z

Lib/test/test_venv.py

-skipInVenv = unittest.skipIf(sys.prefix != sys.base_prefix,
-                             'Test not appropriate in a venv')
+requireVenvCreate = unittest.skipUnless(
+    sys.platform.startswith('win')


Can you please add a comment explaining why tests don't need to be skipped on Windows? Is it a new feature of Python 3.8? Is it the new "wrapper" that you merged recently? Maybe add a link to the bpo which added this wrapper.

zooba · 2019-03-09T13:56:50Z

It changed because I added a new decorator to represent "this test requires venv.create to succeed" as distinct from "this test will never work in a venv". I believe mac can also create new venvs from within a venv, at least for Ned's builds, though I haven't tested. There may be tests that never work in a venv regardless of platform.

As it turns out, the only reason tests fail in a venv is because they try and create a new one, so the original decorator was unused.

Maybe we should have a bug to fix venv.create on Linux? Then we won't need any decorator at all.

vstinner · 2019-03-14T16:58:32Z

Sorry, I don't have the bandwidth to review this PR :-(

zooba · 2019-03-21T16:31:36Z

I improved the skip message, the check, and added a comment by the decorator. Assuming CI passes, I'll merge.

vstinner · 2019-03-21T16:43:34Z

Lib/test/test_venv.py

+requireVenvCreate = unittest.skipUnless(
+    hasattr(sys, '_base_executable')
+    or sys.prefix == sys.base_prefix,
+    'cannot run venv.create from within a venv on this platform')


I don't recall what was the previous message, but this one explains well why the test is skipped. Better than the current message. Thanks.

vstinner

The test_venv.py LGTM.

I don't understand the venv change, but it seems specific to Windows so I trust Steve to know what he does :-)

miss-islington · 2019-03-21T17:04:23Z

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

bedevere-bot · 2019-03-21T17:04:34Z

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

Also fixes venvs from the build directory on Windows. (cherry picked from commit 8bba81f) Co-authored-by: Steve Dower <[email protected]>

bpo-35978: Correctly skips venv tests in venvs

3f4261f

zooba added tests Tests in the Lib/test dir skip news needs backport to 3.7 labels Mar 7, 2019

bedevere-bot added the awaiting merge label Mar 7, 2019

the-knights-who-say-ni added the CLA signed label Mar 7, 2019

zooba requested review from pablogsal and vstinner March 7, 2019 17:27

vstinner reviewed Mar 8, 2019

View reviewed changes

Improve skip message and check for venv.create support

e315394

vstinner reviewed Mar 21, 2019

View reviewed changes

vstinner approved these changes Mar 21, 2019

View reviewed changes

zooba merged commit 8bba81f into python:master Mar 21, 2019

bedevere-bot removed the awaiting merge label Mar 21, 2019

zooba deleted the bpo-35978 branch March 21, 2019 17:04

bedevere-bot removed the needs backport to 3.7 label Mar 21, 2019

miss-islington added a commit that referenced this pull request Mar 21, 2019

bpo-35978: Correctly skips venv tests in venvs (GH-12220)

b0967fe

Also fixes venvs from the build directory on Windows. (cherry picked from commit 8bba81f) Co-authored-by: Steve Dower <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Comments

bpo-35978: Correctly skips venv tests in venvs#12220

bpo-35978: Correctly skips venv tests in venvs#12220
zooba merged 2 commits intopython:masterfrom
zooba:bpo-35978

zooba commented Mar 7, 2019 •

edited by bedevere-bot

Loading

Uh oh!

vstinner commented Mar 8, 2019

Uh oh!

vstinner commented Mar 8, 2019

Uh oh!

vstinner Mar 8, 2019

Uh oh!

vstinner Mar 8, 2019

Uh oh!

zooba commented Mar 9, 2019

Uh oh!

vstinner commented Mar 14, 2019

Uh oh!

zooba commented Mar 21, 2019

Uh oh!

vstinner Mar 21, 2019

Uh oh!

vstinner left a comment

Uh oh!

miss-islington commented Mar 21, 2019

Uh oh!

bedevere-bot commented Mar 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Comments

Conversation

zooba commented Mar 7, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Mar 8, 2019

Uh oh!

vstinner commented Mar 8, 2019

Uh oh!

vstinner Mar 8, 2019

Choose a reason for hiding this comment

Uh oh!

vstinner Mar 8, 2019

Choose a reason for hiding this comment

Uh oh!

zooba commented Mar 9, 2019

Uh oh!

vstinner commented Mar 14, 2019

Uh oh!

zooba commented Mar 21, 2019

Uh oh!

vstinner Mar 21, 2019

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Mar 21, 2019

Uh oh!

bedevere-bot commented Mar 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zooba commented Mar 7, 2019 •

edited by bedevere-bot

Loading