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

pytest==8.3.1 collects tests from Python dependencies within conda environments (regression from 8.2.2) #12652

Closed
4 tasks done
joshuacwnewton opened this issue Jul 22, 2024 · 16 comments · Fixed by #12656
Closed
4 tasks done

Comments

@joshuacwnewton
Copy link

joshuacwnewton commented Jul 22, 2024

  • a detailed description of the bug or problem you are having
  • output of pip list from the virtual environment you are using
  • pytest and operating system versions
    • 8.3.1, multiple different GitHub Actions runners (windows-2019, ubuntu-2022, etc.)
  • minimal example if possible

We have the following setup.cfg file:

[tool:pytest]
addopts = --verbose --show-capture=stderr --tb=native
python_files = testing/**/test_*.py
# NB: norecursedirs is *not* set

When running pytest (with no additional options) in the root of our repository, we see the following behavior:

  • pytest==8.2.2: Only our tests are collected.
  • pytest==8.3.1: Our tests and the tests of our dependencies are collected, leading to many errors.
    • collecting ... collected 367457 items / 408 errors / 100 skipped (Woof!)

Changing our setup.cfg to the following didn't fix the problem either:

[tool:pytest]
addopts = --verbose --show-capture=stderr --tb=native
testpaths = testing
python_files = test_*.py

The fix I used was adding norecursedirs = {name_of_venv}


Since this involves collection in virtual environments, the reason for the regression might be related to changes in:

Notably, we use a conda env with pip packages installed inside, which may subvert the expectations of the changes in the above PR.

@joshuacwnewton joshuacwnewton changed the title pytest==8.3.1 collects tests from Python dependencies within venvs (regression from 8.2.2) pytest==8.3.1 collects tests from Python dependencies within venvs unless norecursedirs is set (regression from 8.2.2) Jul 22, 2024
mguaypaq pushed a commit to spinalcordtoolbox/spinalcordtoolbox that referenced this issue Jul 22, 2024
…rom our dependencies within our `venv` (#4571)

## Description

With `pytest==8.3.1`, pytest will recursively search our `conda`
environment unless we explicitly set `norecursedirs`.

## Related issues/PRs

Fixes #4570.

For upstream details, see:

- pytest-dev/pytest#12652
@bluetech
Copy link
Member

Does the venv directory contain a pyvenv.cfg file?

@joshuacwnewton
Copy link
Author

joshuacwnewton commented Jul 22, 2024

Yes!

# In root of project folder (where tests are located at `./testing`)
master|joshua@monarch:~/repos/spinalcordtoolbox/$ find . -name pyvenv.cfg
./python/envs/venv_sct/pyvenv.cfg

@bluetech
Copy link
Member

bluetech commented Jul 22, 2024

Interesting. Any chance you can poke the _in_venv function https://github.com/pytest-dev/pytest/blob/8.3.1/src/_pytest/main.py#L370 to see why it fails for this path ./python/envs/venv_sct/?

@joshuacwnewton
Copy link
Author

In a Python console:

>>> from pathlib import Path
>>> def _in_venv(path: Path) -> bool:
    """Attempt to detect if ``path`` is the root of a Virtual Environment by
    checking for the existence of the pyvenv.cfg file.
    [https://peps.python.org/pep-0405/]"""
    try:
        return path.joinpath("pyvenv.cfg").is_file()
    except OSError:
        return False
    
>>> import os
>>> os.getcwd()
'/home/joshua/repos/spinalcordtoolbox'
>>> _in_venv(Path("./python/envs/venv_sct/"))
True

Exploring more on my end too... 🤔

@joshuacwnewton
Copy link
Author

Hmm. On my local Linux machine, I get fewer errors. Windows specifically reports many more errors... so I might have to test on a Windows machine. See:

@Hemario
Copy link

Hemario commented Jul 23, 2024

Just chiming in. We run automated tested in a docker container and on that container, we load the conda prefix inside the project folder. We could probably have done this better, but it was working for us in the past.

/projects/projectX: the git repository location
/projects/projectX/conda: the conda prefix

Since version 8.3.1 we notice the same issue as described above. Tests from included packages are being discovered.

With pytest 8.2.2: collected 190 items / 1 deselected / 189 selected
With pytest 8.3.1: collected 85611 items / 157 errors / 1 deselected / 29 skipped / 85610 selected

We solved it by adding to pytest.ini

addopts = 
   --ignore=conda

@RonnyPfannschmidt
Copy link
Member

it seems like checking only for modern venv left us wanting for conda - is there a similar easy check we could do for conda prefixes/envs -- i'll reach out to conda on fosstodon to get input on fixing this, we might have to undo the steamlining for now

@joshuacwnewton joshuacwnewton changed the title pytest==8.3.1 collects tests from Python dependencies within venvs unless norecursedirs is set (regression from 8.2.2) pytest==8.3.1 collects tests from Python dependencies within conda environments (regression from 8.2.2) Jul 23, 2024
@jezdez
Copy link

jezdez commented Jul 23, 2024

For the record, I'm responding to @RonnyPfannschmidt reaching out on Mastodon (https://fosstodon.org/@ossronny/112837662322781627), we'll take a look at this ASAP.

@jezdez
Copy link

jezdez commented Jul 23, 2024

@RonnyPfannschmidt Can you provide more details other than what was said here, which feature was added in 8.3.1 that triggered this?

@jezdez
Copy link

jezdez commented Jul 23, 2024

I've filed conda/conda#14069

@jezdez
Copy link

jezdez commented Jul 23, 2024

Looks like #12544/#12545 is the culprit that reduces virtualenv detection to look for pyvenv.cfg only and effectively removes conda environment detection.

Given that we can't easily retrofit existing conda environments out there, and a significant number of users are using pytest within conda environments, I'd appreciate it, if you could revert the patch, or at least retrofit it with the previous detection mechanism.

@RonnyPfannschmidt
Copy link
Member

as per #12545 (comment) - i'd slightly prefer adding explicit conda detection over reverting - in case thats not easy/feasible quickly we go with a revert

@RonnyPfannschmidt
Copy link
Member

@jezdez so the key question - is there a easy foolproof way to check for conda explicitly - even if a pyvenv.cfg was not yet retrofitted

@jezdez
Copy link

jezdez commented Jul 23, 2024

@jezdez is there a good way to check for conda prior to that change? im happy to roll back for now, but if a consistent bugfix that ensures compatibility with older conda is easy, i'd prefer landing that over a revert dance

The best course of action would probably be checking for path.joinpath("conda-meta", "history").is_file() for now, as that's required to exist for conda envs to use the conda environment revision system (conda list --revisions and conda install --revision REV).

@RonnyPfannschmidt
Copy link
Member

working on a bugfix now, venv detection will be expanded to ("conda-meta", "history")

RonnyPfannschmidt added a commit to RonnyPfannschmidt/pytest that referenced this issue Jul 23, 2024
initially we accidentially detected conda environmnts by just testing too many files
after steamlining we no longer detected conda environments
now we explicitly detect conda environments and test for support
RonnyPfannschmidt added a commit to RonnyPfannschmidt/pytest that referenced this issue Jul 23, 2024
initially we accidentially detected conda environmnts by just testing too many files
after steamlining we no longer detected conda environments
now we explicitly detect conda environments and test for support
RonnyPfannschmidt added a commit that referenced this issue Jul 24, 2024
…c806b499ddbb844753b5c8c4d70a8b98b9d1c3a/pr-12656

[PR #12656/6c806b49 backport][8.3.x] explicitly detect conda envs - fixes #12652
@RonnyPfannschmidt
Copy link
Member

released

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 a pull request may close this issue.

5 participants