Skip to content
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

Fix tests: use explicit syspathinsert #7008

Merged
merged 1 commit into from
Apr 4, 2020

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Apr 3, 2020

Use testdir.syspathinsert() with multiprocessing tests:

  • test_chained_exceptions_no_reprcrash
  • test_exception_handling_no_traceback

This only works currently because _importtestmodule changes sys.path
as a side-effect.

It appears to be only required on Windows though - likely due to the
multiprocessing method used there.

Use `testdir.syspathinsert()` with multiprocessing tests:

- test_chained_exceptions_no_reprcrash
- test_exception_handling_no_traceback

This only works currently because `_importtestmodule` changes `sys.path`
as a side-effect.

It appears to be only required on Windows though - likely due to the
multiprocessing method used there.
@bluetech
Copy link
Member

bluetech commented Apr 3, 2020

Can you explain a bit more what problem this solves? I don't quite follow. The test passes the tmpfile path explicitly to runpytest, why does it need to be in the sys path?

@blueyed
Copy link
Contributor Author

blueyed commented Apr 3, 2020

The tests are crashing on Windows when not leaving "sys.path" mangled (via the side effect).
IIRC multiprocessing requires the module to be importable.
(I've noticed this with blueyed#338)
It might also be a problem with using "importlib" import mode.
It is not strictly required for pytest as of now, but since I've had to debug it using pytest-timeout on CI I've thought it makes sense to do it "properly" already anyway.

@bluetech
Copy link
Member

bluetech commented Apr 3, 2020

Thanks for the explanation. Reading a bit, this is mentioned in the multiprocessing docs, for example the note at the bottom here (IIUC).

So LGTM, though would be nice to have comments above the new lines, like "multiprocessing on Windows requires the main module to be importable.".

@blueyed
Copy link
Contributor Author

blueyed commented Apr 4, 2020

It's more related to the method ("spawn") though, which can also be used on Linux (in theory): https://docs.python.org/3/library/multiprocessing.html#contexts-and-start-methods

Anyway, I think by now that runpytest_inprocess should default to syspathinsert=True - which brings it in line with runpytest_subprocess, which changes PYTHONPATH by default.
(A lot of tests currently only work due to the "pollution" of sys.path via pyimport)

Manipulating sys.path is not thread-safe though (might be an issue with parallel tests), so maybe not a good idea to mess with it by default - and require it to be done explicitly after all?

@blueyed blueyed merged commit 48c9f55 into pytest-dev:master Apr 4, 2020
@blueyed blueyed deleted the fix-tests-upstream branch April 4, 2020 10:33
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.

2 participants