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 7.3.2 changes in behaviour regarding conftest.py and testpaths #11104

Closed
joerick opened this issue Jun 13, 2023 · 8 comments · Fixed by #11125
Closed

Pytest 7.3.2 changes in behaviour regarding conftest.py and testpaths #11104

joerick opened this issue Jun 13, 2023 · 8 comments · Fixed by #11125
Labels
type: regression indicates a problem that was introduced in a release which was working previously

Comments

@joerick
Copy link

joerick commented Jun 13, 2023

In cibuildwheel, we have two test suites - the unit tests at /unit_test and the integration test suite at /test. Both /unit_test and /test are listed in testpaths-

pyproject.toml

#...
[tool.pytest.ini_options]
testpaths = [
    "test",
    "unit_test",
]
#...

We then run either unit_test or test using pytest unit_test/pytest test.
Each unit_test/test dir contains a conftest.py file, which adds some options using parser.addoption. One option that is common to both test suites is --run-podman. Before 7.3.2, this setup seemed to work, we could run both unit tests and integration tests without issue. But on 7.3.2 (perhaps since #10988?) we get the following error:

$ pytest unit_test --run-podman
Traceback (most recent call last):
  File "/Users/joerick/Projects/cibuildwheel/env/bin/pytest", line 8, in <module>
    sys.exit(console_main())
...snip...
  File "/Users/joerick/Projects/cibuildwheel/env/lib/python3.9/site-packages/_pytest/config/__init__.py", line 1143, in pytest_load_initial_conftests
    self.pluginmanager._set_initial_conftests(
  File "/Users/joerick/Projects/cibuildwheel/env/lib/python3.9/site-packages/_pytest/config/__init__.py", line 566, in _set_initial_conftests
    self._try_load_conftest(anchor, namespace.importmode, rootpath)
  File "/Users/joerick/Projects/cibuildwheel/env/lib/python3.9/site-packages/_pytest/config/__init__.py", line 583, in _try_load_conftest
    self._getconftestmodules(anchor, importmode, rootpath)
  File "/Users/joerick/Projects/cibuildwheel/env/lib/python3.9/site-packages/_pytest/config/__init__.py", line 612, in _getconftestmodules
    mod = self._importconftest(conftestpath, importmode, rootpath)
  File "/Users/joerick/Projects/cibuildwheel/env/lib/python3.9/site-packages/_pytest/config/__init__.py", line 660, in _importconftest
    self.consider_conftest(mod)
  File "/Users/joerick/Projects/cibuildwheel/env/lib/python3.9/site-packages/_pytest/config/__init__.py", line 742, in consider_conftest
    self.register(conftestmodule, name=conftestmodule.__file__)
  File "/Users/joerick/Projects/cibuildwheel/env/lib/python3.9/site-packages/_pytest/config/__init__.py", line 488, in register
    ret: Optional[str] = super().register(plugin, name)
  File "/Users/joerick/Projects/cibuildwheel/env/lib/python3.9/site-packages/pluggy/_manager.py", line 115, in register
    hook._maybe_apply_history(hookimpl)
  File "/Users/joerick/Projects/cibuildwheel/env/lib/python3.9/site-packages/pluggy/_hooks.py", line 300, in _maybe_apply_history
    res = self._hookexec(self.name, [method], kwargs, False)
  File "/Users/joerick/Projects/cibuildwheel/env/lib/python3.9/site-packages/pluggy/_manager.py", line 80, in _hookexec
    return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
  File "/Users/joerick/Projects/cibuildwheel/env/lib/python3.9/site-packages/pluggy/_callers.py", line 60, in _multicall
    return outcome.get_result()
  File "/Users/joerick/Projects/cibuildwheel/env/lib/python3.9/site-packages/pluggy/_result.py", line 60, in get_result
    raise ex[1].with_traceback(ex[2])
  File "/Users/joerick/Projects/cibuildwheel/env/lib/python3.9/site-packages/pluggy/_callers.py", line 39, in _multicall
    res = hook_impl.function(*args)
  File "/Users/joerick/Projects/cibuildwheel/test/conftest.py", line 10, in pytest_addoption
    parser.addoption("--run-podman", action="store_true", default=False, help="run podman tests")
  File "/Users/joerick/Projects/cibuildwheel/env/lib/python3.9/site-packages/_pytest/config/argparsing.py", line 104, in addoption
    self._anonymous.addoption(*opts, **attrs)
  File "/Users/joerick/Projects/cibuildwheel/env/lib/python3.9/site-packages/_pytest/config/argparsing.py", line 385, in addoption
    raise ValueError("option names %s already added" % conflict)
ValueError: option names {'--run-podman'} already added

Is this an issue in our configuration, or a bug? Should we no longer use testpaths to list all the test suites?

pip list output
Package                        Version     Editable project location
------------------------------ ----------- ------------------------------------
argcomplete                    1.12.3
attrs                          21.4.0
bashlex                        0.16
black                          23.3.0
bracex                         2.2.1
build                          0.7.0
certifi                        2021.10.8
cffi                           1.15.0
cfgv                           3.3.1
charset-normalizer             2.0.12
cibuildwheel                   2.10.0      /Users/joerick/Projects/cibuildwheel
click                          8.1.2
colorlog                       6.6.0
commonmark                     0.9.1
Deprecated                     1.2.13
distlib                        0.3.4
exceptiongroup                 1.1.1
execnet                        1.9.0
fastcore                       1.4.1
filelock                       3.6.0
flake8                         6.0.0
ghapi                          0.1.19
ghp-import                     2.1.0
html2image                     2.0.1
identify                       2.4.12
idna                           3.3
importlib-metadata             4.11.3
iniconfig                      1.1.1
isort                          5.10.1
Jinja2                         3.1.2
livereload                     2.6.3
Markdown                       3.3.7
MarkupSafe                     2.1.1
mccabe                         0.7.0
mergedeep                      1.3.4
mkdocs                         1.3.1
mkdocs-include-markdown-plugin 2.8.0
mkdocs-macros-plugin           0.7.0
mypy                           1.2.0
mypy-extensions                1.0.0
nodeenv                        1.6.0
nox                            2022.1.7
packaging                      23.1
pathspec                       0.9.0
pep517                         0.12.0
pip                            22.2.2
pip-tools                      6.12.2
platformdirs                   2.5.1
pluggy                         1.0.0
pre-commit                     2.17.0
py                             1.11.0
pycodestyle                    2.10.0
pycparser                      2.21
pyflakes                       3.0.1
PyGithub                       1.55
Pygments                       2.11.2
pyinstrument                   4.3.0
PyJWT                          2.3.0
pymdown-extensions             9.3
PyNaCl                         1.5.0
pyparsing                      3.0.7
pytest                         7.3.2
pytest-forked                  1.4.0
pytest-parallel                0.1.1
pytest-timeout                 2.1.0
pytest-xdist                   2.5.0
python-dateutil                2.8.2
PyYAML                         6.0
pyyaml_env_tag                 0.1
requests                       2.27.1
rich                           12.0.1
ruff                           0.0.265
setuptools                     61.3.1
six                            1.16.0
tblib                          1.7.0
termcolor                      1.1.0
toml                           0.10.2
tomli                          2.0.1
tomli_w                        1.0.0
tornado                        6.1
types-certifi                  2021.10.8.1
types-click                    7.1.8
types-Jinja2                   2.11.9
types-MarkupSafe               1.1.10
types-PyYAML                   6.0.5
types-requests                 2.27.16
types-toml                     0.10.4
types-urllib3                  1.26.11
typing_extensions              4.1.1
urllib3                        1.26.9
virtualenv                     20.14.0
watchdog                       2.1.9
wheel                          0.37.1
wrapt                          1.14.0
zipp                           3.7.0

Xref pypa/cibuildwheel#1518

@RonnyPfannschmidt RonnyPfannschmidt added the type: regression indicates a problem that was introduced in a release which was working previously label Jun 13, 2023
@RonnyPfannschmidt
Copy link
Member

a recent bugfix made a hidden mistake in your conftest layout surface

the basic gist is, that with testpaths, pytest now correctly consider the conf-tests in those root test paths as possible early loaded conftests (to supply addopts & co) in turn making sure that all options are always registred

as far as i understand you previously ran either one or the other parts of the testsuite, thus it was never possible that both conftests where loaded at the same time (which was a bug in pytest)

now that pytest more correctly considers all sources of options, an error pops up as previously you actually absolutely had to register in both places since pytest was not picking up all sources of options for a testsuite

the recommended fix would be to move shared options into a plugin module to list it in the pytest_plugins of the conftests

@joerick
Copy link
Author

joerick commented Jun 13, 2023

thank you for the clarification! that does make sense, it's true that we never seemed to run both test suites together, we'd always do pytest unit_test or pytest test so the config was never perfect.

I'll close this out, as it looks to me that the solution is for us to fix our config.

@joerick joerick closed this as completed Jun 13, 2023
@bluetech
Copy link
Member

testpaths is a fallback for when no arguments are given; if the two directories in your testpaths are incompatible, that means the value doesn't make sense so you should just remove your testpaths.


However, I do think there's an issue in pytest here. testpaths is only supposed to be a fallback for the arguments, however #10988 endowed it with further semantics, which increases the complexity and causes problems such as this one.

I think that if argument paths are to be used for finding initial conftests, then we should use the config.args (the result of choosing between command line args, testpaths, invocation dir) rather than testpaths directly.

@nicoddemus WDYT? (Reopening for discussion)

@bluetech bluetech reopened this Jun 13, 2023
@nicoddemus
Copy link
Member

However, I do think there's an issue in pytest here. testpaths is only supposed to be a fallback for the arguments, however #10988 endowed it with further semantics, which increases the complexity and causes problems such as this one.

You are right, I did not realize that at the time.

think that if argument paths are to be used for finding initial conftests, then we should use the config.args (the result of choosing between command line args, testpaths, invocation dir) rather than testpaths directly.

At first glance seems reasonable indeed.

@blackboxsw
Copy link

blackboxsw commented Jun 13, 2023

Agreed this seems a fundamental change in behavior in how testpaths config is treated. It's an optional fallback only used for test collection when no file paths are provided. But. it now always forces the pytest to load conftests pointed to by the root-level configured testpathseven when a specific file or directory of tests is provided. This breaks a paradigm where separate apps or integration tests versus unittests in a single project may have a different set of test dependencies pulled in by their conftests.

Is it desirable that the solution in #10988 should also treat testpaths only as an optional fallback value when namespace.file_or_dir is unset in _set_initial_conftests instead of always appending the value?

What do we think of something like this?

diff --git a/src/_pytest/config/__init__.py b/src/_pytest/config/__init__.py
index 85d8830e7..fa1924ce5 100644
--- a/src/_pytest/config/__init__.py
+++ b/src/_pytest/config/__init__.py
@@ -546,7 +546,10 @@ class PytestPluginManager(PluginManager):
         )
         self._noconftest = namespace.noconftest
         self._using_pyargs = namespace.pyargs
-        testpaths = namespace.file_or_dir + testpaths_ini
+        testpaths = namespace.file_or_dir
+        if not testpaths:
+            # source testpaths_ini value only when command-line files absent
+            testpaths = testpaths_ini
         foundanchor = False
         for testpath in testpaths:
             path = str(testpath)

If folks believe that new conftest search behavior from #10988 should be retained as-is maybe we can document the testpaths treatment for conftest search path treatment more conspicuously in docs

@RonnyPfannschmidt
Copy link
Member

Thanks for the extra details

I consider this a overreaching bugfix

We should restore part of the old behavior until a major release

We also should ensure all test path related conftests are considered for pytest configuration and addoption for consistency in a major release

@nicoddemus
Copy link
Member

Agreed.

Sorry I won't be able to work on this today (likely tomorrow), so if anybody wants to contribute a fix, it would be greatly appreciated!

@bluetech
Copy link
Member

I can handle this one.

bluetech added a commit to bluetech/pytest that referenced this issue Jun 20, 2023
Fixes pytest-dev#11104.

See the issue for a description of the problem.

Now, we use the same logic for initial conftest paths as we do for
deciding the initial args, which was the idea behind checking
`namespace.file_or_dir` and `testpaths` previously.

This fixes the issue of `testpaths` being considered for initial
conftests even when it's not used for the args.

(Another issue in faeb161 was that the
`testpaths` were not glob-expanded, this is also fixed.)
bluetech added a commit to bluetech/pytest that referenced this issue Jun 21, 2023
Fixes pytest-dev#11104.

See the issue for a description of the problem.

Now, we use the same logic for initial conftest paths as we do for
deciding the initial args, which was the idea behind checking
`namespace.file_or_dir` and `testpaths` previously.

This fixes the issue of `testpaths` being considered for initial
conftests even when it's not used for the args.

(Another issue in faeb161 was that the
`testpaths` were not glob-expanded, this is also fixed.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: regression indicates a problem that was introduced in a release which was working previously
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants