Validate enum-valued configuration options with Literal types - #14791
Draft
Pierre-Sassoulas wants to merge 8 commits into
Draft
Validate enum-valued configuration options with Literal types#14791Pierre-Sassoulas wants to merge 8 commits into
Pierre-Sassoulas wants to merge 8 commits into
Conversation
The option was registered as a plain string and validated by hand in TempPathFactory.from_config with a bare ValueError, which surfaced as an INTERNALERROR traceback during pytest_configure. Register it with the existing RetentionType Literal so config.getini owns the choices, re-raise as UsageError for a clean 'ERROR: ...' report, and drop the manual check. pytest --help and the API reference now show the accepted values, so the '(all/failed/none)' suffix in the help string goes away. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The option was registered as a plain string with an implicit '' default that every consumer normalized to 'skip', validated by a hand-written check in pytest_configure, and get_empty_parameterset_mark ended in a LookupError fallback. Register it with an _EmptyParameterSetMark Literal and an explicit 'skip' default: config.getini owns the choices (re-raised as UsageError at configure time for a clean report), the ''/None tolerance and the LookupError branch become unreachable, and the consumer is an exhaustive match with assert_never like compare_text.py. test_parameterset_for_parametrize_bad_markname smuggled 'bad' through the now Literal-typed valid-values test; it is superseded by test_parameterset_for_parametrize_marks_invalid, which asserts the clean error end to end. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The option was registered as a plain string; an invalid value silently fell back to the classic style in _determine_show_progress_info, and the help string did not mention the 'times' value at all. Register it with a _ConsoleOutputStyle Literal: pytest_configure validates eagerly (re-raised as UsageError for a clean report, since the value is only read lazily during reporting) and the consumer becomes an exhaustive match with an explicit 'classic' case and assert_never instead of the silent else fallback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The option already had clean UsageError validation, but hand-rolled: a get_assertion_text_diff_style wrapper matching the value against ASSERTION_TEXT_DIFF_STYLE_* constants duplicating the _AssertionTextDiffStyle Literal. Register it with the alias so config.getini owns the choices, inline the wrapper at its two call sites (pytest_configure validates eagerly like the other plugins; the per-comparison read is a plain getini of the cached, already-validated value), and drop the constants — mypy checks bare 'ndiff'/'block' literals against the alias, now also in the test helpers that previously carried the value as str. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The option was registered as a plain string with no validation at all: an invalid family passed through LogXML untouched and crashed at report time with a KeyError on the families table, but only once a testcase was recorded. Register it with a _JunitFamily Literal so config.getini rejects invalid values up front (re-raised as UsageError at configure time for a clean report), type the value end to end through LogXML.__init__ and the test helpers, and drop the choice list from the help string now that pytest --help renders it from the type. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The option was registered as a plain string with no validation: an invalid value matched none of the branches in write_captured_output and silently behaved like 'no', with no warning that the requested capture never made it into the report. Register it with a _JunitLogging Literal so config.getini rejects invalid values (re-raised as UsageError at configure time), type the value through LogXML.__init__ and the test helpers, and drop the choice list from the help string now that pytest --help renders it from the type. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The option was registered as a plain string with no validation: an
invalid value matched neither 'total' nor any report phase in
update_testcase_duration, so every testcase silently reported
time="0.000" ('setup' and 'teardown' also worked by accident through
the report.when comparison, undocumented). Register it with a
_JunitDurationReport Literal restricted to the documented values so
config.getini rejects anything else (re-raised as UsageError at
configure time), type the value through LogXML.__init__ and the test
helper, drop the choice list from the help string and the vestigial
'# choices=' comment.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The changelog file is named XXXXX pending the PR number. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pierre-Sassoulas
added
the
type: enhancement
psf-chronographer
Bot
added
the
bot:chronographer:provided
| # Eagerly validate the value; it is only read lazily when an assertion fails. | ||
| try: | ||
| config.getini("assertion_text_diff_style") | ||
| except (TypeError, ValueError) as e: |
Member
There was a problem hiding this comment.
should getini create this usage-error in general?
| else: | ||
| return False | ||
| cfg: _ConsoleOutputStyle = self.config.getini("console_output_style") | ||
| match cfg: |
Member
There was a problem hiding this comment.
im wondering if we may rely on type checkers exthausiveness checks here in some way
but this may need some addition we cant have in this pr
| f"tmp_path_retention_policy must be either all, failed, none. Current input: {policy}." | ||
| ) | ||
| try: | ||
| policy: RetentionType = config.getini("tmp_path_retention_policy") |
Member
There was a problem hiding this comment.
im of the impression that practically all cases of this need to transform to usage-error - so i believe its a good idea to just make it that way by default
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
2 participants
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.
Follow up to #14751, which added type expressions to
Parser.addini, and to the review of #14692 which asked for an audit of the other configuration options. Better reviewed commit by commit.Register the seven options that have a fixed set of valid values with a
Literaltype, so the accepted values live in the registration, are validated byconfig.getini, and show up inpytest --helpand the API referenceInvalid values now fail with
ERROR: <inipath>: config option '...' expects one of ..., got '...'(exit code 4). Configure-time reads re-raise asUsageErrorbecause a rawValueErrorfrom apytest_configurehook surfaces as an INTERNALERROR traceback.Minor behavior changes:
empty_parameter_set_marknow defaults to"skip"explicitly; an empty value is rejected instead of normalized to skipjunit_duration_reportno longer accepts the undocumentedsetup/teardownvalues that worked by accident