Test suite immediately aborts with a successful exit code whenever a test that does a pipenv run executes

[jfly]

Check this out:

$ time pytest
=============================================== test session starts ================================================
platform linux -- Python 3.8.10, pytest-6.2.5, py-1.11.0, pluggy-1.0.0
rootdir: /home/jeremy/src/github.com/pypa/pipenv, configfile: setup.cfg, testpaths: tests
plugins: timeout-2.0.1, xdist-2.4.0, flaky-3.7.0, forked-1.3.0, pypi-0.1.1
collected 274 items

tests/integration/test_cli.py ...
real	7.14s
user	4.30s
sys	0.73s
$ echo $?
0

I know we don't have a 7 second test suite with only 3 tests in it! Also, where did pipenv's normal test summary go?

In the example above, we've run the first 3 tests of test_cli.py and the test suite aborted when running the 4th test: test_pipenv_site_packages. The whole test suite aborts when we get to this pipenv run invocation:

pipenv/tests/integration/test_cli.py

Line 52 in b0ebaf0

c = p.pipenv('run python -c "import sysconfig; print(sysconfig.get_path(\'stdlib\'))"')

This doesn't happen in CI. It took me forever to figure out what's going on here. Here's the full story:

• That call to p.pipenv uses click's invoke method internally. That method internally starts executing pipenv (note that this is happening in the same python testrunner process, there is no forking going on here!)
• Eventually we get into the actual guts of pipenv run, which (on my Linux machine) means we execute this call to os.execve in do_run_posix. This completely replaces the pytest process we were in the middle of, and when the (now just pipenv) process exits, our entire test suite grinds to a halt and exits successfully. Yikes 🤯!
• This doesn't happen in CI because we actually use do_run_nt (:interrobang:) in CI:

  pipenv/pipenv/core.py

  Lines 2499 to 2502 in b0ebaf0

  	if os.name == "nt" or environments.PIPENV_IS_CI:
  	run_fn = do_run_nt
  	else:
  	run_fn = do_run_posix

  . It looks like @frostming added this in https://github.com/pypa/pipenv/pull/4757/files#diff-ef852c4ac364f946819f765a6bc26f04f1b0968f31fc512949a60fa2ab0685e8R2488, presumably as a workaround for this very problem?
• (For the record, if you're running pytest with parallelism enabled, you'll see error messages like "node down: Not properly terminated" and "replacing crashed worker gw3" as the workers turn into pipenv processes and exit).

I've been able to work around this problem by explicitly setting the CI environment variable when running tests locally. I'll send in a PR to improve the documentation, but we might want to rework this a bit. It feels a bit off to require people to pretend to be in CI just to run tests.

[matteius]

@jfly I haven't experienced this issue before in the last months of running pipenv tests locally on almost daily basis. Could you re-check this behavior with pipenv==2022.4.21?

[jfly]

@matteius that's because of this workaround I added. If you comment it out, this reproduces quickly:

$ git diff
diff --git a/tests/conftest.py b/tests/conftest.py
index 13778403..1dbfcfab 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -1,14 +1,14 @@
 import pytest
-import os
-
-# Note that we have to do this *before* `pipenv.environments` gets imported,
-# which is why we're doing it here as a side effect of importing this module.
-# CI=1 is necessary as a workaround for https://github.com/pypa/pipenv/issues/4909
-os.environ['CI'] = '1'
-
-def pytest_sessionstart(session):
-    import pipenv.environments
-    assert pipenv.environments.PIPENV_IS_CI
+# <<< import os
+# <<<
+# <<< # Note that we have to do this *before* `pipenv.environments` gets imported,
+# <<< # which is why we're doing it here as a side effect of importing this module.
+# <<< # CI=1 is necessary as a workaround for https://github.com/pypa/pipenv/issues/4909
+# <<< os.environ['CI'] = '1'
+# <<<
+# <<< def pytest_sessionstart(session):
+# <<<     import pipenv.environments
+# <<<     assert pipenv.environments.PIPENV_IS_CI
 
 
 @pytest.fixture()
$ time pytest
===================================================================== test session starts =====================================================================
platform linux -- Python 3.8.10, pytest-6.2.5, py-1.11.0, pluggy-1.0.0
rootdir: /home/jeremy/src/github.com/pypa/pipenv, configfile: setup.cfg, testpaths: tests
plugins: timeout-2.0.1, xdist-2.4.0, flaky-3.7.0, forked-1.3.0, pypi-0.1.1
collected 294 items

tests/integration/test_cli.py ...
real	9.78s
user	6.02s
sys	0.74s

[matteius]

@jfly sorry for being dense but is this still an issue?

[jfly]

@matteius sorry for the delayed response here. I just checked, and yes, this is still an issue. I've filed this SO question seeking help from the click folks about how to handle this: https://stackoverflow.com/questions/73995157/best-practice-for-invoke-ing-a-tool-that-internally-calls-exec.

[scvsoft-javierrotelli]

@matteius that's because of this workaround I added. If you comment it out, this reproduces quickly:

Hi!
This workaround breaks process substitution in CI.

https://github.com/pypa/pipenv/blob/main/pipenv/routines/shell.py#L102

we have a script that runs something like
pipenv run ./python_cli --file <(echo $FILE_CONTENT)
this works fine on local machines, but it breaks on CI with: Path '/dev/fd/63' does not exist.

this is because when the $CI env var is set, run uses do_run_nt instead of do_run_posix. do_run_nt uses a subprocess, so the temp file descriptor created with <(..) is not accesible when it runs.

[jfly]

Urg, sorry @scvsoft-javierrotelli, that must have been a pain to track down.

One solution would be to change that workaround to look at a new environment variable (PIPENV_FORCE_RUN_NT?) that we explicitly set in Pipenv's CI config.

[matteius]

Its been a while on this thread but I am wondering how relevant that pipenv logic for nt is still, but it might be -- anyway I am pretty busy at the moment but could accept a PR and cut a minor release sometime soon.

[scvsoft-javierrotelli]

a new env variable seems like a good solution, I can work on a PR between today and tomorrow if that's fine

[scvsoft-javierrotelli]

Urg, sorry @scvsoft-javierrotelli, that must have been a pain to track down.

One solution would be to change that workaround to look at a new environment variable (PIPENV_FORCE_RUN_NT?) that we explicitly set in Pipenv's CI config.

heh, It was hard to find out. but a good exercise nonetheless

[jfly]

I am wondering how relevant that pipenv logic for nt is still

I don't know anything about pipenv on windows, but I am pretty confident the workaround described in this issue is still necessary. The fact that the string "nt" is part of this is pretty confusing, it's just about if we're using click.invoke or a subprocess incantation.

I can work on a PR between today and tomorrow if that's fine

@scvsoft-javierrotelli, thanks! I'm not a maintainer (so I won't be able to merge your changes), but I can give you a code review.

[matteius]

@jfly the only test that fails in the CI without any of that legacy patch is an oddly written test test_scripts -- I actually think we maybe can address it by removing the patch (like in my PR and revisiting the test logic of test_scripts)

[matteius]

@jfly and the test that failed was literally checking if not os nt then compare to the wrong string for not found (it was could not be found) "not found" != "not be found" -- anyway, if you want to take a look at the PR i'll add a news fragment later and then I think @scvsoft-javierrotelli won't need to open anything for this issue.

[jfly]

@matteius I hadn't realized the click.invoke stuff is now gone (c715b11#r138087532)! Yes, I believe this can get cleaned up now.