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

Tests using virtual environments: pass the copy of existing env to the subprocesses #3133

Merged
merged 3 commits into from
Feb 28, 2022

Conversation

befeleme
Copy link
Contributor

@befeleme befeleme commented Feb 24, 2022

Summary of changes

In specifically set environment like the build environment in Fedora, we noticed some of the tests running subprocesses were failing. Typically they didn't see the correct paths, leading to multiple ModuleNotFoundError: No module named setuptools and failed meta-tests in test_virtualenv (DID NOT RAISE...).
The changes: passing the copy of the environment minus PYTHONPATH for the subprocesses made the test suite pass during our builds.

Pull Request Checklist

  • Changes have tests (I'd say N/A - those are test fixes)
  • News fragment added in changelog.d/.
    (See documentation for details)

@abravalheri
Copy link
Contributor

abravalheri commented Feb 24, 2022

@befeleme thank you very much for working on this.

There are somethings I don't understand: in the tests the venvs are supposed to have their own installation of setuptools, and testing a complete independent installation of setuptools after it is packaged, is also important...

When looking here:

cmd = [self.exe(cmd[0])] + cmd[1:]

and in jaraco.envs, the environment object seems to do the right thing to find the correct executable inside the virtual env...

When looking at:

env.req = str(setuptools_wheel)

env.create_opts = ['--no-setuptools']

and here, and here, I cannot understand how setuptools is not getting installed when it should (you mentioned ModuleNotFoundError: No module named setuptool), or why setuptools is getting installed when it should not not (you mentioned DID NOT RAISE...)...

Can you elaborate a bit more on how have you reached the conclusion that missing env vars are the root cause of this problem?
Do you know what would be the missing variables, and how did they influence the correct creation of the virtual environment used inside the tests.

@abravalheri abravalheri added the Needs Investigation Issues which are likely in scope but need investigation to figure out the cause label Feb 24, 2022
@befeleme
Copy link
Contributor Author

befeleme commented Feb 24, 2022

Firstly, thank you for taking time to look at this PR.

Let me elaborate on what happens with the current 60.9.3 when I run the test suite in the build environment for Fedora.
My understanding so far is that it's not about setuptools being or not being installed in venv, but about skewed visibility of modules if PYTHONPATH is set.
If you can point to a better explanation and fix, it would be much appreciated.

We set PYTHONPATH for the test run in the build environment to /builddir/build/BUILD/setuptools-60.9.3. The package was already built and installed to this directory and we want the test suite to run against the installed package.

Problem 1
When test_pip_upgrade_from_source runs the invocation of the meta-test, this happens in venv.run:

cmd is set to [Path('/tmp/pytest-of-mockbuild/pytest-33/test_pip_upgrade_from_source_p3/venv_without_setuptools/.env/bin/python'), '-c', 'import setuptools']
PYTHONPATH is set to /builddir/build/BUILD/setuptools-60.9.3

Test tries to import setuptools and it succeeds, and my understanding is that the installed-by-fedora-build package is indeed on PYTHONPATH and that's where 'DID NOT RAISE...' failure comes from.
If I unset PYTHONPATH before invoking the import, "global" setuptools stops being visible from the inside of the venv and the test passes as the exception is raised.

Problem 2
As for

  • test_distutils_local and test_distutils_local_with_setuptools from test_distutils_adoption.py
  • test_test_command_install_requirements from test_virtualenv.py

the situation looks a bit different.
The first two tests check whether the .env is present in distutils.__file__.

test_distutils_local doesn't find .env in the path.

>       assert venv.name in find_distutils(venv, env=env).split(os.sep)
E       AssertionError: assert '.env' in **['', 'usr', 'lib64', 'python3.10', 'distutils', '__init__.py\n']**

test_distutils_local_with_setuptools can't import setuptools, test_test_command_install_requirements can't import pkg_resources.

Traceback (most recent call last):
  File "<string>", line 1, in <module>
ModuleNotFoundError: No module named 'setuptools'

But if env.create() is run in the context unsetting PYTHONPATH, the magic happens and the assertions are true as .env is present in both paths:

/tmp/pytest-of-mockbuild/pytest-39/test_distutils_local_with_setu0/venv/.env/lib/python3.10/site-packages/setuptools/_distutils/__init__.py
['', 'tmp', 'pytest-of-mockbuild', 'pytest-40', 'test_distutils_local0', 'venv', '.env', 'lib', 'python3.10', 'site-packages', 'setuptools', '_distutils', '__init__.py\n']

I suspect that copy of the environment that takes place here duplicates "our" PYTHONPATH and causes essentially the same problem with visibility as in the first issue.

I hope this shows where we're being hit and why this approach was taken to fix it (I'm sorry for the wall of text, though!)

@abravalheri
Copy link
Contributor

abravalheri commented Feb 24, 2022

Thank you very much @befeleme for the explanation. Nice to see that we are starting to get to the bottom of the problems :)

Please correct me here if I am wrong, If I understand it correctly this problem will always happen if someone, instead of running the tests via tox, uses PYTHONPATH to point to a specific setuptools codebase, right? Once PYTHONPATH is set, it will leak to the spawned processed when env is not explicitly passed.

I always think that making tests more isolated is a good thing, so removing items from the environment variables that are not explicitly set should be fine!

However, I would avoid removing items from the env dictionary when it is explicitly set. For whatever reason, tests might need to set PYTHONPATH, so I would like to avoid surprises for the developers.

setuptools/tests/environment.py Outdated Show resolved Hide resolved
setuptools/tests/environment.py Outdated Show resolved Hide resolved
setuptools/tests/fixtures.py Outdated Show resolved Hide resolved
@abravalheri
Copy link
Contributor

abravalheri commented Feb 24, 2022

@befeleme there are somethings that are still not obvious to me, could you try to help me understand?

test_distutils_local doesn't find .env in the path
test_distutils_local_with_setuptools can't import setuptools, test_test_command_install_requirements can't import pkg_resources.

Assuming that implicit PYTHONPATH is removed from the environment variables in the subprocess1, why would these tests fail? The venv is supposed to have an installation of setuptools from the wheel/sdist right? And SETUPTOOLS_USE_DISTUTILS="local" should prevent distutils from being loaded from the standard library...

I suspect that copy of the environment that takes place here duplicates "our" PYTHONPATH and causes essentially the same problem with visibility as in the first issue.

It is also not clear to me how having PYTHONPATH set while venv is being created will affect the way it runs afterwards2

Footnotes

  1. In test_distutils_adoption.py, the env dictionary seems to be explicitly set (e.g. env = dict(SETUPTOOLS_USE_DISTUTILS='local')), so in this case PYTHONPATH should not be leaking... (It might only be leaking in test_pip_import and test_distutils_has_origin...)
    In test_test_command_install_requirements there is already a contexts.environment(PYTHONPATH=None, ...) preventing the leak...

  2. Maybe if --system-site-packages was passed to virtualenv, but that is not the case, right?

@befeleme
Copy link
Contributor Author

Please correct me here if I am wrong, If I understand it correctly this problem will always happen if someone, instead of running the tests via tox, uses PYTHONPATH to point to a specific setuptools codebase, right? Once PYTHONPATH is set, it will leak to the spawned processed when env is not explicitly passed.

Yes, this is what happens.

However, I would avoid removing items from the env dictionary when it is explicitly set. For whatever reason, tests might need to set PYTHONPATH, so I would like to avoid surprises for the developers.

Yes, this is possible. I've run the test suite on a modified patch, when I only touch environment which hasn't been explicitly set by test and the tests pass. Which puzzled me at first, but then given the venv is created and installs package in a context unsetting PYTHONPATH, this makes sense, as it's already removed from the way.

Regarding the second problem, I think @hroncok's clarification better explains the point I've missed.

I'll send the updated (and better explained) patch soon. Thank you for the well-pointed questions!

@abravalheri
Copy link
Contributor

abravalheri commented Feb 25, 2022

Thank you very much @befeleme! Sorry for disturbing you with a lot of questions.

@abravalheri abravalheri added enhancement and removed Needs Investigation Issues which are likely in scope but need investigation to figure out the cause labels Feb 25, 2022
@befeleme
Copy link
Contributor Author

I have just sent the updated patch. The comments are quite verbose but I hope for a good clarity of the explanation (and I've never managed to keep it short, eh).

Sorry for disturbing you with a lot of questions.

No, it's great! This discussion helped me understand the issue much deeper. :)

@befeleme
Copy link
Contributor Author

The last force-push invalidated the approved workflow. @abravalheri, could you please rerun it? I'm ready with the updates.

Copy link
Contributor

@abravalheri abravalheri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much @befeleme, that is a very great work, and followed by an in depth-investigation!

setuptools/tests/fixtures.py Outdated Show resolved Hide resolved
setuptools/tests/environment.py Outdated Show resolved Hide resolved
@abravalheri
Copy link
Contributor

@Mergifyio rebase

@mergify
Copy link
Contributor

mergify bot commented Feb 27, 2022

rebase

✅ Branch has been successfully rebased

@abravalheri
Copy link
Contributor

I am sorry @befeleme, I was hopping that 5ae9aa4 would have fixed the problems with the CI but apparently we still have other problems to solve in the main branch.

Maybe this PR will take a bit longer to land (until we fix the problems in the main).

@jaraco
Copy link
Member

jaraco commented Feb 27, 2022

@Mergifyio rebase

befeleme and others added 3 commits February 27, 2022 20:39
This enhances environment isolation, as in special cases, like downstream
distro packaging, PYTHONPATH can be set to point to a specific setuptools
codebase. When it leaks, it shadows the virtual environment's paths and
produces wrong test results.
@mergify
Copy link
Contributor

mergify bot commented Feb 27, 2022

rebase

✅ Branch has been successfully rebased

@abravalheri abravalheri merged commit e309995 into pypa:main Feb 28, 2022
@befeleme befeleme deleted the fix-tests-for-fedora branch March 9, 2022 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants