fix: validate marker names in -m expression with --strict-markers#14127
fix: validate marker names in -m expression with --strict-markers#14127RonnyPfannschmidt wants to merge 4 commits intopytest-dev:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request implements validation of marker names in -m (marker expression) arguments when --strict-markers is enabled, addressing issue #2781. Previously, --strict-markers only validated markers used in test decorations (e.g., @pytest.mark.foo), but not markers referenced in command-line -m expressions.
Changes:
- Added tracking of identifier names during expression parsing to capture marker names used in expressions
- Implemented validation of marker names in
-mexpressions against registered markers when strict mode is enabled - Added comprehensive test coverage for the new validation behavior
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/_pytest/mark/expression.py | Modified Scanner, expression parser, and Expression class to track and expose identifier names used in expressions |
| src/_pytest/mark/init.py | Added _validate_marker_names() function to validate markers in expressions when strict_markers is enabled |
| testing/test_mark_expression.py | Added unit tests for the new Expression.idents() method |
| testing/test_mark.py | Added integration test verifying that unregistered markers in -m expressions trigger errors with --strict-markers |
| changelog/2781.feature.rst | Documented the new feature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
testing/test_mark.py
Outdated
| pytester.makeini( | ||
| """ | ||
| [pytest] | ||
| markers = registered: a registered marker | ||
| """ | ||
| ) |
There was a problem hiding this comment.
The makeini call here is redundant because it's immediately overwritten by another makeini call on lines 239-245 when option_name is "strict_markers" or "strict". This first ini file is never used. Consider removing these lines or restructuring the test to avoid creating the ini file twice.
ec4ad16 to
20e1063
Compare
bluetech
left a comment
There was a problem hiding this comment.
I debated with myself whether this is a good feature. It can help with bad typos, but maybe there's a use case for trying non-registered markers? But I couldn't think of any such use cases. So LGTM. Let's see if anyone complains...
Consider also mentioning this briefly in the strict_markers documentation.
changelog/2781.feature.rst
Outdated
| @@ -0,0 +1 @@ | |||
| When ``--strict-markers`` is enabled, marker names used in ``-m`` expressions are now validated against registered markers. | |||
There was a problem hiding this comment.
| When ``--strict-markers`` is enabled, marker names used in ``-m`` expressions are now validated against registered markers. | |
| When :confval:`strict_markers` is enabled, marker names used in :option:`-m` expressions are now validated against registered markers. |
src/_pytest/mark/__init__.py
Outdated
|
|
||
| # Get registered markers from the ini configuration. | ||
| registered_markers: set[str] = set() | ||
| for line in config.getini("markers"): |
There was a problem hiding this comment.
We currently have this in 2 places (in _pytest.mark.pytest_cmdline_main and in MarkGenerator. If we are to follow the "rule of three", maybe can add a method to Config like
def _iter_registered_markers(self) -> Iterator[tuple[str, str]]
which yields (name, description) (I don't remember if the description is optional, if it is, would be tuple[str, str | None]).
src/_pytest/mark/__init__.py
Outdated
| if not strict_markers: | ||
| return | ||
|
|
||
| # Get registered markers from the ini configuration. |
There was a problem hiding this comment.
Personally I don't find this comment adds much, but OK if it helps.
There was a problem hiding this comment.
the commments are sometihng i need to retrain myself to note and remove
src/_pytest/mark/__init__.py
Outdated
| marker = line.split(":")[0].split("(")[0].strip() | ||
| registered_markers.add(marker) | ||
|
|
||
| # Check each identifier in the expression. |
There was a problem hiding this comment.
Personally I don't find this comment adds much, but OK if it helps.
src/_pytest/mark/expression.py
Outdated
| return ret | ||
| ident = s.accept(TokenType.IDENT) | ||
| if ident: | ||
| # This is a marker/keyword name - track it. |
There was a problem hiding this comment.
Personally I don't find this comment adds much, but OK if it helps.
src/_pytest/mark/expression.py
Outdated
| #: The original input line, as a string. | ||
| self.input: Final = input | ||
| self._code: Final = code | ||
| #: The identifiers (marker/keyword names) used in the expression. |
There was a problem hiding this comment.
Expression is written to be "independent" of its uses, i.e. it doesn't "know" about markers or keywords. I would therefore remove the parenthetical here.
src/_pytest/mark/expression.py
Outdated
| """Return the set of identifier names used in the expression. | ||
|
|
||
| For marker expressions (-m), these are marker names. | ||
| For keyword expressions (-k), these are keyword names. | ||
| """ |
There was a problem hiding this comment.
Same here, I suggest dropping the mention of marker/keyword.
| """Return the set of identifier names used in the expression. | |
| For marker expressions (-m), these are marker names. | |
| For keyword expressions (-k), these are keyword names. | |
| """ | |
| """Return the set of all identifiers which appear in the expression.""" |
20e1063 to
56688fb
Compare
Closes pytest-dev#2781 Co-authored-by: Cursor AI <ai@cursor.sh> Co-authored-by: Anthropic Claude Opus 4.5 <claude@anthropic.com>
56688fb to
2192ac2
Compare
Extract marker line parsing into a single _iter_registered_markers() method on Config class, replacing duplicate parsing logic in three locations: - MarkGenerator._markers cache population - _validate_marker_names() for strict marker checking - pytest_cmdline_main() for --markers display Introduces RegisteredMarker NamedTuple with name, signature, and description fields for cleaner API. Co-authored-by: Cursor AI <ai@cursor.sh> Co-authored-by: Anthropic Claude Opus 4.5 <claude@anthropic.com>
|
|
||
| Yields :class:`RegisteredMarker` named tuples with ``name``, | ||
| ``signature``, and ``description`` fields. |
There was a problem hiding this comment.
Redundant to list the namedtuple fields, as this will tend to get out of sync.
| Yields :class:`RegisteredMarker` named tuples with ``name``, | |
| ``signature``, and ``description`` fields. |
| ``signature``, and ``description`` fields. | ||
| """ | ||
| for line in self.getini("markers"): | ||
| # Example lines: "skipif(condition): skip the given test if..." |
Co-authored-by: Bruno Oliveira <nicoddemus@gmail.com>
for more information, see https://pre-commit.ci
Fixes #2781
When
--strict-markersis enabled, marker names used in-mexpressions are now validated against registered markers.