refactor: dispatch config file loading by filename, then suffix - #14807
Open
RonnyPfannschmidt wants to merge 5 commits into
Open
refactor: dispatch config file loading by filename, then suffix#14807RonnyPfannschmidt wants to merge 5 commits into
RonnyPfannschmidt wants to merge 5 commits into
Conversation
2 tasks
Knowledge about config files was spread across three places that had to be kept in sync by hand: load_config_dict_from_file dispatched on suffix with filename checks nested inside those branches, locate_config re-declared the discovery order in its own hardcoded list, and adding a format meant editing both in the right order. Introduce two tables instead. CONFIG_LOADERS maps a config file *name* to its loader and doubles as the discovery order used by locate_config, so order and parsing can no longer drift apart. CONFIG_SUFFIXES maps a *suffix* to a loader for files passed explicitly via -c/--config-file, which may be named anything. load_config_dict_from_file now looks up the name first and falls back to the suffix. The per-format rules move out of nested conditionals into named functions -- _parse_pytest_ini, _parse_ini_file, _parse_cfg_file, _parse_pytest_toml and _parse_pyproject_toml -- each documenting the rule it implements. This is a pure refactor: no test needed changing, and running load_config_dict_from_file over a matrix of every supported name crossed with present/absent/empty/malformed sections (plus unsupported and extension-less files) yields identical values, modes, origins and exceptions before and after. Two pre-existing warts are deliberately preserved rather than fixed here: the CFG_PYTEST_SECTION message still names setup.cfg for any .cfg file, and a scalar `pytest` key still raises AttributeError. This redoes the structural half of pytest-dev#8358 on top of current main; that PR bundled it with the abandoned setup.cfg deprecation from pytest-dev#3523. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Consolidates three open reports that all bottom out in how an explicitly given config file is located and parsed. The preceding refactor turns each of them into a small change rather than another special case in a conditional. Custom TOML files read [pytest], not [tool.pytest] (pytest-dev#14705) [tool.pytest] is reserved for the project file. A TOML file passed via -c now reads its configuration from [pytest] directly, exactly like pytest.toml does. Previously any .toml that was not pytest.toml or .pytest.toml was parsed with pyproject.toml semantics, so the documented [pytest] table in a custom file was silently ignored. This is one entry in CONFIG_SUFFIXES. -c/--config-file validates its argument (pytest-dev#14716) Previously an invalid path either silently produced an empty configuration -- while still reporting `configfile:` in the header -- or crashed with a raw FileNotFoundError traceback, depending on its extension. Now a path that does not exist, a directory, and a regular file pytest has no loader for are each a UsageError. The supported extensions in the message are derived from CONFIG_SUFFIXES rather than restated in a separate constant. This is breaking for invocations that passed an unparsable file to -c and relied on it being ignored. The pytest-dev#14683 regression test did exactly that with a conftest.py and now uses a real config file; it passes --rootdir explicitly, so the config file was incidental to what it covers. The rootdir is not derived from a non-regular config file (pytest-dev#11502) --config-file=/dev/null is a common way to load no configuration at all. Deriving the rootdir from its parent made the rootdir /dev, and the cache plugin then warned on every run that it could not create /dev/.pytest_cache. Such a path says nothing about where the project lives, so fall back to the usual common-ancestor logic. The diagnoses come from pytest-dev#14707 (@DebadityaHait), pytest-dev#14723 (@wanxiankai) and pytest-dev#14671 (@apoorvdarshan); the implementations differ because the table-based dispatch makes each one smaller. Closes pytest-dev#14705 Closes pytest-dev#14716 Closes pytest-dev#11502 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Passing -c/--config-file set the rootdir to the config file's parent directory. A config kept in a subdirectory -- the common `-c config/pytest.ini` layout -- therefore moved the rootdir into that subdirectory, which broke conftest discovery and node ids: conftests collected an empty path prefix, so same-named fixtures from sibling directories shadowed each other and node ids came out as `config::test2` instead of `tests/tests2/test2.py::test2`. The config file's location is evidence about where the project lives, not a decision on its own. Take the common ancestor of the config file's directory and the collected test paths instead, falling back to the invocation directory in place of the test paths when none were given. If the two live in unrelated trees the ancestor degrades to the filesystem root, in which case the config file's directory is kept, as before. This covers cases neither of the two open proposals handled alone: scenario before pytest-dev#14454 pytest-dev#14579 now -c config/pytest.ini (no args) config/ ok config/ ok -c config/pytest.ini tests/ config/ ok ok ok sibling config, invoked from a subdirectory config/ sub/ ok ok unrelated working directory config/ unrel. ok ok pytest-dev#14454 (@EternalRights) found the root cause and drove the design discussion; its `invocation_dir` rule is right for the no-args case but follows the working directory even when that is unrelated to the project. pytest-dev#14579 (@hariharan077) contributed the common-ancestor form, which is most of the answer but leaves the no-args case unfixed. Neither is needed once the ancestor also considers the invocation directory, and no ambiguity warning is required because the ancestor is derived rather than guessed. Closes pytest-dev#13246 Closes pytest-dev#9703 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
RonnyPfannschmidt
force-pushed
the
refactor/config-file-dispatch
branch
from
July 31, 2026 08:34
1878bb4 to
b7cc13f
Compare
RonnyPfannschmidt
marked this pull request as ready for review
July 31, 2026 08:53
The new determine_setup branches had three paths with no test: the is_fs_root fallback for a regular config file whose directory shares no meaningful ancestor with the collected paths, the same fallback for a config file that is not a regular file, and --rootdir taking precedence over a non-regular config file. Each asserts the behaviour rather than merely reaching the line: unrelated trees keep the config file's directory, rootless arguments fall back to the invocation directory, and an explicit --rootdir still wins. The remaining uncovered lines in findpaths.py are the Python 3.10 tomli import -- exercised by the py310 CI jobs -- and two paths that predate this branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Supersedes #8358, #14707, #14723, #14671, #14454 and #14579.
Closes #14705
Closes #14716
Closes #11502
Closes #13246
Closes #9703
Three commits: a behaviour-preserving refactor of config file dispatch, then the fixes it makes small, then the rootdir fix. Review them separately — the first is intended to be a pure refactor; the second and third deliberately change behaviour (including one breaking change, flagged below).
Commit 1: refactor config file dispatch
Re-does the structural part of #8358 on top of current
main. That PR bundled the refactor with thesetup.cfgdeprecation from #3523; the deprecation was abandoned (and reverted within the branch), and the refactor — which @nicoddemus had signed off as "Overall the changes look good" — went down with it. This PR carries only the structural change. No deprecation, no policy change, no behaviour change.It can't be cherry-picked: the 2021 branch predates
ConfigValue, native TOML mode, ini option aliases,pytest.toml/.pytest.toml, andignored_config_files. So this is a re-implementation of the same design against today's code.The problem
Knowledge about config files is currently spread across three places that must be kept in sync by hand:
load_config_dict_from_filedispatches on suffix (if filepath.suffix == ".ini" / elif ".cfg" / elif ".toml"), with filename checks nested inside those branches (if filepath.name in {"pytest.ini", ".pytest.ini"},if filepath.name in ("pytest.toml", ".pytest.toml")).locate_configre-declares the discovery order in a separate hardcodedconfig_nameslist.The name-vs-suffix distinction is real and load-bearing —
pytest.iniis config even when empty, a random.iniis not;pyproject.tomlreads[tool.pytest],pytest.tomlreads[pytest]— but it is implicit and interleaved rather than expressed.The change
Two tables, and dispatch that reads off them:
locate_confignow iteratesCONFIG_LOADERSdirectly instead of maintaining its own list, so discovery order and parsing can no longer drift apart.Per-format semantics move into named functions —
_parse_pytest_ini,_parse_ini_file,_parse_cfg_file,_parse_pytest_toml,_parse_pyproject_toml— each with a docstring stating its rule, instead of living in nested conditionals inside one ~100-line function.Behaviour preservation
This is intended to be a pure refactor.
load_config_dict_from_fileover a 38-case matrix (every supported name × present/absent/empty/malformed section, plus.txtand extension-less files) undermainand under this branch, comparing returned values,ConfigValue.mode/origin, and raised exception types and messages. Output is byte-identical.Two spots deliberately keep a pre-existing wart rather than quietly fixing it, so the diff stays behaviour-preserving:
_parse_cfg_filestill hardcodessetup.cfgin theCFG_PYTEST_SECTIONmessage even for other.cfgfiles._parse_pytest_toml/_parse_pyproject_tomlstill assume thepytestkey is a table; a scalar there raisesAttributeErroras before (hence thetype: ignorecomments). Worth hardening, separately.In this commit
CONFIG_SUFFIXES[".toml"]still points at_parse_pyproject_toml, which is whatmaindoes today, so that the refactor decides nothing. Commit 2 changes it — that is #14705.Why now
Three open PRs are each adding another special case to exactly the conditionals this removes, and two of them conflict:
-c) is the name-vs-suffix confusion itself. On this branch it becomes a one-line change — pointCONFIG_SUFFIXES[".toml"]at a_parse_pytest_toml-style loader — with no risk topyproject.tomldiscovery. As currently written it inverts a nested check tofilepath.name != "pyproject.toml", i.e. dispatch by negation.SUPPORTED_CONFIG_FILE_EXTENSIONS— a fourth hardcoded table of file knowledge. Here that set isCONFIG_SUFFIXES.keys(), derived rather than declared.determine_setupbranch in opposite directions: fix: validate explicit config file paths #14723 rejects any config path whereis_file()is false, Do not derive rootdir from a non-regular config file #14671 exists to support--config-file=/dev/null(a character device, sois_file()is false). Whichever lands second silently undoes the other.Landing this first turns those three into small, independent, non-conflicting changes — which is what commit 2 does.
Commit 2: the three fixes the refactor enables
fix: validate -c/--config-file and read [pytest] from custom TOML filesConsolidates three open reports that all bottom out in how an explicitly given config file is located and parsed:
[tool.pytest]instead of[pytest]CONFIG_SUFFIXES[".toml"]now points at_parse_pytest_toml-chas two failure modes for invalid pathsdetermine_setuprejects nonexistent paths, directories, and regular files with no loaderCONFIG_SUFFIXES--config-file=/dev/nullgivesrootdir: /devis_file()The diagnoses come from #14707 (@DebadityaHait), #14723 (@wanxiankai) and #14671 (@apoorvdarshan) — this PR supersedes all three, with smaller implementations because the dispatch tables carry the file knowledge.
Notably #14723 and #14671 are mutually exclusive as written: #14723 rejects any config path where
is_file()is false, while #14671 exists to support--config-file=/dev/null, a character device for whichis_file()is false. Splitting the check into exists (error if not) and is a regular file (parse it, and only then derive the rootdir from it) satisfies both.Passing a file pytest has no loader for to
-c/--config-fileis now aUsageErrorinstead of being silently ignored. pytest's own #14683 regression test did this with aconftest.py; it now uses a real config file, and since it passes--rootdirexplicitly the config file was incidental to what it covers.Verification
Full suite green (4337 passed, 47 skipped, 13 xfailed, 7 xpassed — the xpasses are pre-existing, #11603/#10042). Each reported reproducer checked by hand as well: the custom-TOML case is confirmed by
python_filesfrom the custom file actually taking effect, and the/dev/nullcase by the rootdir landing on the project directory with noPytestCacheWarning.Commit 3: the config file's directory no longer decides the rootdir alone
-cset the rootdir to the config file's parent. With the common-c config/pytest.inilayout that moved the rootdir intoconfig/, so conftests collected an empty path prefix: same-named fixtures from sibling directories shadowed each other and node ids came out asconfig::test2rather thantests/tests2/test2.py::test2.The config file's location is evidence about where the project lives, not a decision on its own. Take the common ancestor of the config file's directory and the collected test paths, using the invocation directory in their place when no paths were given. If the two live in unrelated trees the ancestor degrades to the filesystem root, in which case the config file's directory is kept, as today.
-c config/pytest.ini(no args)config/config/-c config/pytest.ini tests/config/config/sub/config/This supersedes both open proposals:
invocation_dirrule is right for the no-args case but follows the working directory even when that is unrelated to the project. Its ambiguity warning is also written toParser.extra_info, which is only rendered byPytestArgumentParser.error()— so it never appears on a successful run, and its tests pass by asserting onconfig._parser.extra_infodirectly.if dirsand so leaves the no-args case unfixed.No ambiguity warning is needed once the ancestor also considers the invocation directory, because the rootdir is then derived rather than guessed.
Verified against the original reproducers by swapping in
main'sfindpaths.pyand running the same commands: onmainthe #13246 repro givesFAILED config::test2 - assert 1 == 2androotdir: .../config; with this commit, 2 passed androotdir: .../— the project root.