feat: support keyword arguments in marker expressions #12500

lovetheguitar · 2024-06-20T16:50:09Z

With this change you can restrict a test run to only run tests matching one or multiple marker
keyword arguments
Only keyword argument matching is supported in marker expressions.
Only int, str, bool & None values are supported in marker expressions.

webknjaz · 2024-06-21T12:18:01Z

@lovetheguitar could you rebase the PR, fill out the PR description/commits, and compose a change note?

src/_pytest/mark/__init__.py

lovetheguitar · 2024-06-21T14:58:03Z

@RonnyPfannschmidt Could you please have a look? :)

lovetheguitar · 2024-06-21T16:12:37Z

@webknjaz https://readthedocs.org/projects/pytest/builds/24773128/ does the preview fail to link that or did I reference it wrongly?

Edit:

The solution was to define a "title" when referencing since at the anchor there was no explicit headline.

:ref:`marker examples <marker_keyword_expression_example>`

Thanks for the tip @webknjaz.

webknjaz

I mostly reviewed this promptly. The walrus thing seems to be the most important blocker.

changelog/12281.feature.rst

AUTHORS

changelog/12281.feature.rst

doc/en/example/markers.rst

doc/en/how-to/usage.rst

src/_pytest/mark/__init__.py

lovetheguitar

Should be good to go @RonnyPfannschmidt. 🚀 🤞

doc/en/how-to/usage.rst

src/_pytest/mark/__init__.py

doc/en/example/markers.rst

changelog/12281.feature.rst

AUTHORS

Fixes pytest-dev#12281

…pressions

RonnyPfannschmidt · 2024-06-21T19:57:03Z

src/_pytest/mark/__init__.py

+        if not (matches := self.own_mark_name_mapping.get(name, [])):
+            return False
+
+        if not kwargs:


This case cannot happen, I'll fix it when I get back to the computer, but feel free to beat me to it

That was an early return to keep the logic for "non-keyword" markers fast/similar to before.

The case happens every time a "non-keyword" marker is looked up

pytest/src/_pytest/mark/expression.py

Lines 271 to 272 in f75e3fe

def __bool__(self) -> bool:

return self.matcher(self.name)

changelog/12281.feature.rst

doc/en/example/markers.rst

src/_pytest/mark/__init__.py

Addressed

RonnyPfannschmidt · 2024-06-21T21:03:24Z

awesome - @lovetheguitar thanks for seeing this tough and @webknjaz thanks for sorting out the last details to land this

The-Compiler

I think this is amazing work, and it's great how many tests there are and corner cases there are handled!

I'm really unhappy how this was merged in a seemingly unfinished state (TODO comments in various places), while we were eating dinner together, 2 hours after the final version was pushed. I found a couple of pretty obvious things from a cursory review, and now I'll either need to do a follow-up PR to pick up the lose ends, or someone else needs to go back to opening a new PR and all that additional work.

Also, @bluetech wrote the parser in the first place if memory serves me right, so I feel like it'd be appropriate to give him a chance to look at this, no?

The-Compiler · 2024-06-21T21:07:27Z

doc/en/example/markers.rst

+
+.. code-block:: pytest
+
+    $ pytest -v -m 'device(serial="123")'


Single quotes won't work on Windows, which is why we use double quotes above.

In poweshell they do, but good to know they don’t in cmd.

✅ Changed to ".

The-Compiler · 2024-06-21T21:07:48Z

doc/en/how-to/usage.rst

+
+.. code-block:: bash
+
+    pytest -m slow(phase=1)


Needs quotes to work.

✅ Added " around the marker expression.

The-Compiler · 2024-06-21T21:16:10Z

testing/test_mark.py

@@ -233,6 +233,54 @@ def test_two():
    assert passed_str == expected_passed


+@pytest.mark.parametrize(
+    ("expr", "expected_passed"),
+    [  # TODO: improve/sort out


Leftover TODO?

Those were the first tests added, any feedback greatly appreciated. Otherwise I can just drop the todo.

Preliminarily dropped.

The-Compiler · 2024-06-21T21:17:56Z

testing/test_mark_expression.py

+        ("mark(empty_list=[])", r'unexpected character/s "\[\]"'),
+    ),
+)
+def test_invalid_kwarg_name_or_value(  # TODO: move to `test_syntax_errors` ?


Leftover TODO?

Yep, sorry, forgot to remove that one, talked about it with Ronny to keep it separate as is.

✅ Removed.

The-Compiler · 2024-06-21T21:18:03Z

testing/test_mark_expression.py

+        ("builtin_matchers_mark(z=1)", False),
+    ),
+)
+def test_builtin_matchers_keyword_expressions(  # TODO: naming when decided


Leftover TODO?

Same for that one. 🙃

✅ Removed.

lovetheguitar · 2024-06-21T22:27:42Z

I think this is amazing work, and it's great how many tests there are and corner cases there are handled!

I'm really unhappy how this was merged in a seemingly unfinished state (TODO comments in various places), while we were eating dinner together, 2 hours after the final version was pushed. I found a couple of pretty obvious things from a cursory review, and now I'll either need to do a follow-up PR to pick up the lose ends, or someone else needs to go back to opening a new PR and all that additional work.

Also, @bluetech wrote the parser in the first place if memory serves me right, so I feel like it'd be appropriate to give him a chance to look at this, no?

@The-Compiler Sure thing, I was also thinking about mentioning @bluetech, since, yes he wrote that part and he commented also in the issue.

Feel free to be nitpicky, I'm motivated to learn the customs properly and do the right thing (TM). 😄

bluetech

Very nice work, I wouldn't have done it better myself :)

I left some comments.

bluetech · 2024-06-22T07:22:09Z

changelog/12281.feature.rst

+
+Now tests can be selected by marker keyword arguments.
+Supported values are :class:`int`, (unescaped) :class:`str`, :class:`bool` & :data:`None`.
+


It would be nice to add an example here, for people skimming the changelog without clicking through to the link.

I had one in there in the beginning. 😅 I think @RonnyPfannschmidt suggested to remove it and link to the docs.

Do I add it back here or leave as is?

@lovetheguitar I think, it's nice to have examples in such places, but perhaps only small ones. I like Ran's idea. If you make a PR for that, we could discuss something more concrete there, with dedicated review and polishing.

@bluetech I believe the previous comment of @lovetheguitar was addressed to you, eh?

bluetech · 2024-06-22T07:33:17Z

src/_pytest/mark/expression.py

@@ -5,7 +5,8 @@
 expression: expr? EOF
 expr:       and_expr ('or' and_expr)*
 and_expr:   not_expr ('and' not_expr)*
-not_expr:   'not' not_expr | '(' expr ')' | ident
+not_expr:   'not' not_expr | '(' expr ')' | ident ( '(' name '=' value ( ', ' name '=' value )*  ')')*


Would remove the space after ident ( for consistency.

The * (zero-or-more) at the end of this line should be ? (zero-or-one), I think.

The definitions of the name and value productions are missing.

removed the space

true, changed to ?

added a definition, not sure about the semantics though 😅

bluetech · 2024-06-22T07:34:04Z

src/_pytest/mark/expression.py

@@ -5,7 +5,8 @@
 expression: expr? EOF
 expr:       and_expr ('or' and_expr)*
 and_expr:   not_expr ('and' not_expr)*
-not_expr:   'not' not_expr | '(' expr ')' | ident
+not_expr:   'not' not_expr | '(' expr ')' | ident ( '(' name '=' value ( ', ' name '=' value )*  ')')*
+
 ident:      (\w|:|\+|-|\.|\[|\]|\\|/)+

 The semantics are:


Let's describe here the semantics of ident(...).

Added, tried to adhere to the previous style. LMKWYT.

bluetech · 2024-06-22T07:36:23Z

src/_pytest/mark/expression.py

@@ -43,6 +47,9 @@ class TokenType(enum.Enum):
    NOT = "not"
    IDENT = "identifier"
    EOF = "end of input"
+    EQUAL = "="
+    STRING = "str"


"str" might be confusing, I suggest "string literal"

✅ I agree, changed accordingly and added test case.

webknjaz · 2024-06-22T19:42:45Z

Too bad that it got auto-merged earlier, than intended. I knew that Ronny didn't want to make the process too difficult for a first-time contributor so I fixed a few concerns myself and hit Approve to replace the blocking review, assuming that Ronny did check the other things. And I addressed Ronny's comment and dismissed his review, communicating with him offline. This was in line with Ronny's plan and the auto-merge that he'd set earlier (it wasn't auto-reset after my commits). I should've instead dismissed my review so it'd not be a blocker and not hit approve, as there were parts I didn't really inspect deeply.

@lovetheguitar since you stated you wanted to do better, here are some action items that you could perform (with an attempt to explain why they are important):

Make a follow-up PR or several of them, addressing the comments that Ran and Florian made (I suppose, this one is rather obvious);
When you see examples of things similar to what you're adding, it's usually best to follow the surrounding style — this avoids additional mental overhead for the reviewers since the pre-existing pieces have been battle-tested and any customizations would need to be inspected from various angles and might be missed by some reviewers (I missed it, for example, and didn't even know it was significant);
I usually recommend not closing/dismissing the separate inline review threads started by someone else for the following reasons:
- only the thread creator knows if you actually addressed their concerns — you can guess but in general, the only way of really knowing is to let them declare the thing solved. A review is a multi-sided conversation and not someone throwing in instructions to be either followed or dismissed silently — be verbose about what's going on and let others get onto the same page with you;
- clicking the Resolve conversation button silently adds mental overhead for the creator since it doesn't generate any notifications and just becomes collapsed; It then requires someone to click on all the collapsed threads, trying to figure out where that old context exists and attempt to judge whether it was solved and guess why it disappeared as the reason is not always obvious and see the previous point for why it might not actually be resolved. It creates more round-trips, confusion, and frustration. I recommend just re-asking if you feel like it might be resolved or waiting;
- in the previous point, I mentioned that no-notification disappearance of threads. To solve this point, if you're like really sure you can decide that it's resolved by yourself (for example, when you followed what was requested to the letter), I suggest writing a summary/notification comment about this action — I typically explain why/how the thing was resolved and add explicit text like "Marking this thread as resolved." This lets other people actually find it by following their email notifications and registering that they don't need to follow that conversation anymore. I've also seen great pull request authors including commit links there — in case they didn't click on the Commit suggestion button and re-implemented it.

That point (3) is something that happened here — I suggested a few correct fixes and extra explanations in some places. It was possible to click one button on the GH UI and get them in. However, they ended up being dismissed for a mysterious reason that I still have no idea of. There was a comment about choosing to use an alternative (worse) syntax option but the motivation was never stated. So I still had to just add those things manually which prolonged the PR cycle — because of lack of communication.
And with the other suggestion, it was dismissed with an incorrect perception, forcing me to dig the thread from the pile of others and unresolve it.

To summarize, the PR was good in general, but a few things were missed during the review and the transparency could be better (not just on the PR author's side, though).

lovetheguitar · 2024-06-22T20:49:10Z

Very nice work, I wouldn't have done it better myself :)

I left some comments.

Thanks for the fast review @bluetech. I tried to address the open points in #12523.
Looking forward to some feedback.

Improve documentation & other kinks of marker keyword expression PR #12500

lovetheguitar force-pushed the feat/support_marker_kwarg_in_marker_expressions branch 2 times, most recently from 9451472 to ac18898 Compare June 20, 2024 21:10

webknjaz reviewed Jun 21, 2024

View reviewed changes

src/_pytest/mark/__init__.py Show resolved Hide resolved

webknjaz reviewed Jun 21, 2024

View reviewed changes

src/_pytest/mark/__init__.py Outdated Show resolved Hide resolved

webknjaz reviewed Jun 21, 2024

View reviewed changes

src/_pytest/mark/__init__.py Outdated Show resolved Hide resolved

webknjaz reviewed Jun 21, 2024

View reviewed changes

src/_pytest/mark/__init__.py Outdated Show resolved Hide resolved

lovetheguitar force-pushed the feat/support_marker_kwarg_in_marker_expressions branch from e49f06a to c672f9f Compare June 21, 2024 14:31

psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Jun 21, 2024

lovetheguitar force-pushed the feat/support_marker_kwarg_in_marker_expressions branch from c672f9f to 7d4f437 Compare June 21, 2024 14:43

lovetheguitar force-pushed the feat/support_marker_kwarg_in_marker_expressions branch 3 times, most recently from 3e0ebe6 to edbbbaf Compare June 21, 2024 16:06

lovetheguitar force-pushed the feat/support_marker_kwarg_in_marker_expressions branch from edbbbaf to 11c9435 Compare June 21, 2024 16:21

webknjaz requested changes Jun 21, 2024

View reviewed changes

lovetheguitar force-pushed the feat/support_marker_kwarg_in_marker_expressions branch from 11c9435 to 2b960c3 Compare June 21, 2024 18:36

lovetheguitar commented Jun 21, 2024

View reviewed changes

lovetheguitar force-pushed the feat/support_marker_kwarg_in_marker_expressions branch from 2b960c3 to ae6c7d4 Compare June 21, 2024 18:50

lovetheguitar added 10 commits June 21, 2024 20:51

feat: support keyword arguments in marker expressions

15c33fb

Fixes pytest-dev#12281

style: use @overload to get rid of mypy only assertions

04f457c

test: add empty string keyword argument marker test cases

1cc35ec

refactor(mark): use existing NOT_SET sentinel

f489739

perf(expression): improve string lexing & error messages

1e7eb20

refactor(MarkMatcher): replace from_item with from_markers method

6dd8ad6

test: use new MarkMatcher.from_markers method & register test markers

3921d94

test(test_mark.py): add sad case that -k doesn't support keyword ex…

7c7c36d

…pressions

docs: document keyword argument support in marker expressions

598d881

docs(12281.feature.rst): add changelog fragment

9cf9cfa

lovetheguitar added 2 commits June 21, 2024 20:51

docs(AUTHORS): add myself as contributor

c3e8983

style(mark): type hint **kwargs as str | int | bool | None

b1255a9

lovetheguitar force-pushed the feat/support_marker_kwarg_in_marker_expressions branch from ae6c7d4 to b1255a9 Compare June 21, 2024 18:51

RonnyPfannschmidt enabled auto-merge June 21, 2024 18:53

RonnyPfannschmidt approved these changes Jun 21, 2024

View reviewed changes

RonnyPfannschmidt previously requested changes Jun 21, 2024

View reviewed changes

webknjaz reviewed Jun 21, 2024

View reviewed changes

changelog/12281.feature.rst Outdated Show resolved Hide resolved

webknjaz reviewed Jun 21, 2024

View reviewed changes

doc/en/example/markers.rst Outdated Show resolved Hide resolved

webknjaz added 2 commits June 21, 2024 22:05

📝 Use explicit RST roles for built-in types in docs

24450e3

📝 Drop stray trailing period from the change note byline

329662e

webknjaz reviewed Jun 21, 2024

View reviewed changes

src/_pytest/mark/__init__.py Outdated Show resolved Hide resolved

🔥 Drop the missing kwargs case

75a2225

webknjaz requested review from RonnyPfannschmidt and webknjaz June 21, 2024 20:10

webknjaz approved these changes Jun 21, 2024

View reviewed changes

RonnyPfannschmidt merged commit 9f134fc into pytest-dev:main Jun 21, 2024
29 checks passed

The-Compiler reviewed Jun 21, 2024

View reviewed changes

bluetech reviewed Jun 22, 2024

View reviewed changes

lovetheguitar mentioned this pull request Jun 22, 2024

Improve documentation & other kinks of marker keyword expression PR #12500 #12523

Merged

The-Compiler added a commit that referenced this pull request Jun 25, 2024

Merge pull request #12523 from lovetheguitar/fix/follow_up_to_pr_#12500

53bf188

Improve documentation & other kinks of marker keyword expression PR #12500


		Now tests can be selected by marker keyword arguments.
		Supported values are :class:`int`, (unescaped) :class:`str`, :class:`bool` & :data:`None`.

feat: support keyword arguments in marker expressions #12500

feat: support keyword arguments in marker expressions #12500

Conversation

lovetheguitar commented Jun 20, 2024 • edited Loading

webknjaz commented Jun 21, 2024

lovetheguitar commented Jun 21, 2024

lovetheguitar commented Jun 21, 2024 • edited Loading

webknjaz left a comment

Choose a reason for hiding this comment

lovetheguitar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RonnyPfannschmidt commented Jun 21, 2024

The-Compiler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lovetheguitar commented Jun 21, 2024

bluetech left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webknjaz commented Jun 22, 2024

lovetheguitar commented Jun 22, 2024

lovetheguitar commented Jun 20, 2024 •

edited

Loading

lovetheguitar commented Jun 21, 2024 •

edited

Loading