Enable parallel testing on windows and update psutil#3049
Conversation
htgoebel
left a comment
There was a problem hiding this comment.
The code looks okay, but the commits are a mess. This are about 25 commit for just a few changes, I do not even see what and why this was changed.
Please clean this up to be in a state for merging. Please also mind the Commit message format. Thanks.
PyInstaller/compat.py
Outdated
| # See http://bugs.python.org/issue12029 for more details | ||
| return exctype is not None and issubclass(exctype, self._exceptions) | ||
| else: | ||
| from contextlib import suppress |
There was a problem hiding this comment.
This is used in the test suite only, right? Then it should go into e.g. PyInstaller/utils/tests.py or a new module PyInstaller/utils/tests_compat.py
There was a problem hiding this comment.
It's currently used in the test suite, but the idea is to include it for future use elsewhere in PyInstaller.
| # Monkeypatch the psutil subprocess on Python 2 | ||
| if is_py2: | ||
| import subprocess32 | ||
| psutil.subprocess = subprocess32 |
There was a problem hiding this comment.
Please state in a comment message why this is required.
There was a problem hiding this comment.
Python 2.7 doesn't have subprocess.communicate(timeout). subprocess32 does. And since we're only using it for the tests, we don't need to worry about the implications of including this dependency.
tests/functional/conftest.py
Outdated
| process = psutil.Popen(args, executable=exe_path, stdout=sys.stdout, | ||
| stderr=sys.stderr, env=prog_env, cwd=prog_cwd) | ||
| process = psutil.Popen(args, executable=exe_path, stdout=subprocess.PIPE, | ||
| stderr=subprocess.PIPE, env=prog_env, cwd=prog_cwd) |
There was a problem hiding this comment.
What is the idea behind this change?
There was a problem hiding this comment.
This is the other one of two problems. For some reason (as I explained in the previous PR that attempted this), the subprocess module will "irritate" the stdout handle by making various C API calls to the underlying handle (which is nonstandard/provided by pytest), such that it closes non-deterministically. Once that happens, the subprocess will die when it can no longer write to stdout or stderr.
| pytest >= 2.7.3 # Testing framework. | ||
| pytest-xdist # Plugin allowing running tests in parallel. | ||
| pytest-timeout # Plugin to abort hanging tests. | ||
| pytest-catchlog # Capture log output separately from stdout/stderr |
There was a problem hiding this comment.
Why? I was distracted by the logs written out, too, but are the other reasons? And the reason should be stated in teh commit message.
There was a problem hiding this comment.
IC:
pytest-catchlog plugin has been merged into the core, please remove it from your requirements.
There was a problem hiding this comment.
This is in fact what makes this PR possible. pytest-catchlog was one of two problems that impaired parallel testing. It's been merged into pytest core.
|
Another question: Are these changes related or are they dependent? |
|
WRT to commits, I agree that they are a mess. They will be cleaned up eventually. However, the code can be reviewed now. |
The testing change depends on the |
|
Actually we can just drop catchlog for 3.3. Restarting the tests to see if it works now. |
The suppress function is handy way to avoid
try:
something
except:
pass
Adding it allows using the new suppress idom
with suppress(Exception):
something
which is much cleaner. This allows use of the
new function elsewhere in PyInstaller, although
it is initially used in the tests.
The plugin caused issues with parallel tests on windows, and is no longer needed except on Python 3.3 (which I think we can all agree that it is not an issue if the log output is not captured on Python 3.3, as it will be dropped shortly).
Here, we enable parallel testing on windows by working around a bug in the pytest framework: if the sys.stdout is passed to a subprocess, it is randomly closed. Therefore, we buffer the output in memory and then write it ourselves. As the subprocess output is not really that verbose (compared to the log output), this is not an issue. In addition, we patch subprocess32 into psutil on Python 2.7 so that we can call communicate(timeout). It's a hack, but it's only used for testing, so there should be no broader implications.
|
@htgoebel This should be ready to merge and I'd rather do it sooner than later to speed the windows CI. If there are failures, it's probably due to @tallforasmurf. |
|
Very good commit-messaged. Thanks! |
As usual, the commits are absolutely horrible, but I will clean them up eventually. Feel free to force push to this branch and clean them up if you have time.