Skip to content

Conversation

gomri15
Copy link

@gomri15 gomri15 commented Oct 3, 2025

Added a public function which evaluates markers matching

  • Introduced match_markexpr to match marker expressions against markers or pytest Items.
  • Updated documentation to reflect the new function.
  • Added comprehensive tests to ensure functionality and error handling.

close: #13759

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Oct 3, 2025
@gomri15 gomri15 force-pushed the 13759-public-interface-for-matchmarker branch from 705cd94 to b7f2edb Compare October 3, 2025 13:53
…valuation

- Introduced match_markexpr to match marker expressions against markers or pytest Items.
- Updated documentation to reflect the new function.
- Added comprehensive tests to ensure functionality and error handling.

Closes pytest-dev#13737
@gomri15 gomri15 force-pushed the 13759-public-interface-for-matchmarker branch from b7f2edb to f87cc2b Compare October 3, 2025 14:59
Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I won't get to do a deep review this weekend however direct concenience for only exactly your usecase is not the api we should expose

The costs there are all wrong for anything but single use

Its not appropriate for something like usage in modifyitems

@gomri15
Copy link
Author

gomri15 commented Oct 3, 2025

Thanks for having a look at this so quickly.

could you clarify a few things for me so I can get to work in the right direction.

  • how does this public API relate modifyitems? from my understanding of what you wrote there is some connection between the two which I am unclear about

  • What minimal API shape would you be comfortable exposing? If you have a preferred example of intended usage, I’ll match that.

  • could you elaborate on what costs you are referring to?

if I understand you correctly, your concern is that if the code was used in this state in modifyitems? the result would not be optimal?

@RonnyPfannschmidt
Copy link
Member

a key use case for the matching is something like

def deselect_by_mark(items: list[Item], config: Config) -> None:
matchexpr = config.option.markexpr
if not matchexpr:
return
expr = _parse_expression(matchexpr, "Wrong expression passed to '-m'")
remaining: list[Item] = []
deselected: list[Item] = []
for item in items:
if expr.evaluate(MarkMatcher.from_markers(item.iter_markers())):
remaining.append(item)
else:
deselected.append(item)
if deselected:
config.hook.pytest_deselected(items=deselected)
items[:] = remaining

which requires a loop over all items with the matcher

exposing the functionality as one off one by one will sabotage users from using it like that

we have to have a discussion about a good api to expose that includes parsing and preparing match dicts

@RonnyPfannschmidt
Copy link
Member

@nicoddemus @bluetech some input needed

i was thinking we might want to expose Expression under the name MarkExpression

and then expand it with evaluate_markers(item_or_markers: Item|Sequence[Markfoo]) as well as evaluate_keywords(item: Item)
that way we are able to create a parsed expression and use it once or multiple times

additionally i'd recommend to add the _pytest_private hack to its ctor and keep the input sequence around for the repr/debugging

@bluetech
Copy link
Member

bluetech commented Oct 6, 2025

Not exposed to exposing Expression, can be good and I can see how it can be useful outside. I went over its code now and made a few tweaks #13790.

I don't think the name MarkExpression is appropriate, since it also matches keywords (-k) which are not marks.

and then expand it with evaluate_markers(item_or_markers: Item|Sequence[Markfoo]) as well as evaluate_keywords(item: Item)

I wouldn't want to do this, currently Expression is designed to be independent of a concrete matching logic. Instead we should expose the "matchers" that are passed to expression.evaluate() if it makes sense to do so (didn't check).

@RonnyPfannschmidt
Copy link
Member

The idea for the exposed api includes some considerations for convenience as for general consumption the additional exposure of the matchers for marks/keyeords and their consequent usage seems cumbersome

If its for internal usage the separation is great as thr usage points are limited

But the developer experience for that api is a bit cumbersome

@gomri15
Copy link
Author

gomri15 commented Oct 7, 2025

The idea for the exposed api includes some considerations for convenience as for general consumption the additional exposure of the matchers for marks/keyeords and their consequent usage seems cumbersome

If its for internal usage the separation is great as thr usage points are limited

But the developer experience for that Api is a bit cumbersome

@RonnyPfannschmidt
If I am understanding you correctly you want to avoid complying expression multiple times when comparing to multiple marks or string etc...

and that would make, the general goal an API which allows the user to complie and evaluate expressions.

That all being true than the idea of exposing the class Expression seems most simple and straightforward.

The risks being as I see it that this is an integral part of the project which we are now exposing which could causes issues in future if we want to refactor or change it significantly.

@RonnyPfannschmidt
Copy link
Member

I believe you overestimate the impact of providing expression

Yes it will require some care however it's not deeply embedded

@gomri15
Copy link
Author

gomri15 commented Oct 7, 2025

@RonnyPfannschmidt ok, thanks for clarifying.
Unless you respond otherwise i will start working on this

@RonnyPfannschmidt
Copy link
Member

@gomri15 we should discuss in what way we expose the matchers currently in use and what convenience methods may be sensible to make the developer experience good

CC @bluetech

@gomri15
Copy link
Author

gomri15 commented Oct 7, 2025

@RonnyPfannschmidt I guess that all comes down to the expected usage of such APIs?.

in my mind it makes sense someone would want to check if a mark they passed in -m is going to collect all the tests they expect it to collect and maybe fail if not so they don't run their whole CI while missing critical tests.

@RonnyPfannschmidt
Copy link
Member

Please show where your usecase would happen

@gomri15
Copy link
Author

gomri15 commented Oct 7, 2025

@RonnyPfannschmidt not sure what you mean but if I understand you correctly, you're asking for an example of where the use case I am suggesting would happen.

i was speaking in general about possible use cases for this public API since I don't have any actual use cases apart from the one the initial requester for of this API mentioned, so I was theorizing a use case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided (automation) changelog entry is part of PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Public way to test if a matchexpr will match a marker string or Item
3 participants