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

Include PYTHONPATH in get_env_with_venv_bin for use in subprocesses #4052

Merged

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented May 6, 2020

Fixes #4051

The test tests.cmdline.commands.test_process:test_pause_play_kill
was failing in local setups with a time out. The problem was that the
daemon, launched as a subprocess in the setUp, would fail to load the
submitted process, because the class could not be imported. This is
because it is defined in the tests.utils module, which would not be
part of the Python path of the subprocess.

The tests module is automatically added to the path by pytest,
however, it was not being forwarded to the subprocess. Passing the
current environment through the env argument solves this. The only
additional change is to include the PYTHONPATH variable in the env
copy created by aiida.cmdline.utils.common.get_env_with_venv_bin.
These two changes guarantee that the test process classes are importable
by the daemon that is running in the subprocess.

@sphuber
Copy link
Contributor Author

sphuber commented May 6, 2020

Only thing that I am not sure about is that this change will also be used in non-test code, since we use the get_env_with_venv_bin function also in normal use. Should we put a switch in to only include the python path when running tests? Normally, these switches tend to be very dirty. Trying to think if it is bad to set the PYTHONPATH for the subprocess from the main. In principle that is the goal of this get_env_with_venv_bin function. We want to make sure that daemon runners have the same environment. So I don't think this should be problematic

@codecov
Copy link

codecov bot commented May 6, 2020

Codecov Report

Merging #4052 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #4052   +/-   ##
========================================
  Coverage    78.45%   78.45%           
========================================
  Files          461      461           
  Lines        34077    34077           
========================================
  Hits         26731    26731           
  Misses        7346     7346           
Flag Coverage Δ
#django 70.49% <ø> (ø)
#sqlalchemy 71.35% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f7ac506...8320f99. Read the comment docs.

…ill`

The test `tests.cmdline.commands.test_process:test_pause_play_kill`
was failing in local setups with a time out. The problem was that the
daemon, launched as a subprocess in the `setUp`, would fail to load the
submitted process, because the class could not be imported. This is
because it is defined in the `tests.utils` module, which would not be
part of the Python path of the subprocess.

The `tests` module is automatically added to the path by `pytest`,
however, it was not being forwarded to the subprocess. Passing the
current environment through the `env` argument solves this. The only
additional change is to include the `PYTHONPATH` variable in the env
copy created by `aiida.cmdline.utils.common.get_env_with_venv_bin`.
These two changes guarantee that the test process classes are importable
by the daemon that is running in the subprocess.
@sphuber sphuber force-pushed the fix/4051/tests-module-python-path branch from 1e4cc8d to 8320f99 Compare May 6, 2020 12:21
@sphuber
Copy link
Contributor Author

sphuber commented May 6, 2020

After discussion with @giovannipizzi we opt to only apply the change to the test itself, until we properly understand the full effect of always including the PYTHONPATH when copying the env.

@sphuber sphuber merged commit d39ec6a into aiidateam:develop May 6, 2020
@sphuber sphuber deleted the fix/4051/tests-module-python-path branch May 8, 2020 10:30
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.

Timeout in unit test tests.cmdline.commands.test_processes.py:test_pause_play_kill when running locally
2 participants