Emit warning on unknown marks via decorator #4933
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4933 +/- ##
==========================================
- Coverage 95.68% 92.25% -3.44%
==========================================
Files 113 113
Lines 25178 25174 -4
Branches 2498 2498
==========================================
- Hits 24092 23224 -868
- Misses 767 1605 +838
- Partials 319 345 +26
Continue to review full report at Codecov.
|
|
|
||
| Marks added by pytest or by a plugin instead of the decorator will not trigger | ||
| the warning or this error. Test suites that want to enforce a limited set of | ||
| markers can add ``--strict`` to ``addopts``: |
There was a problem hiding this comment.
Can we introduce --strict-marks as an alias to --strict?
Historically --strict started with marks but it was intended to also turn warnings into errors, but those plans have changed now that we use standard warnings, so I think it makes sense to make that explicit in the option name. Thoughts?
There was a problem hiding this comment.
i like to see --strict go away as the current form is simply very lacking
There was a problem hiding this comment.
I'd prefer to leave that to a future PR, since I don't have time to do that too at the moment.
| @@ -9,6 +9,14 @@ class PytestWarning(UserWarning): | |||
| """ | |||
|
|
|||
|
|
|||
| class UnknownMarkWarning(PytestWarning): | |||
There was a problem hiding this comment.
About creating a new warning, see my thoughts on the bottom of #4830 (comment).
cc @blueyed
If we go down that route, we should introduce warning types for the other PytestWarning types we produce (I think there are just a few).
There was a problem hiding this comment.
If we go down that route, we should introduce warning types for the other PytestWarning types we produce (I think there are just a few).
Just to be clear: if we decide to add more warning types, we should do it in a separate PR so as not to burden this one.
There was a problem hiding this comment.
OK, taking it out of this one.
| @@ -187,6 +187,9 @@ filterwarnings = | |||
| # Do not cause SyntaxError for invalid escape sequences in py37. | |||
| default:invalid escape sequence:DeprecationWarning | |||
| pytester_example_dir = testing/example_scripts | |||
| markers = | |||
| issue | |||
| pytester_example_path | |||
There was a problem hiding this comment.
This marker should be registered by pytester I think
| @@ -284,27 +285,48 @@ def test_function(): | |||
|
|
|||
| _config = None | |||
|
|
|||
| # These marks are always treated as known for the warning, which is designed | |||
There was a problem hiding this comment.
I suppose the proper way to handle this is to make each plugin to register their known marks, just like any plugin (for example skipping.py should register skip, skipif and xfail markers).
There was a problem hiding this comment.
🤦♂️ They already do, actually. That'll simplify the implementation a bit...
|
Looking good so far! Please see my comments. 👍 |
|
This should target features. I've noticed strange output for failures: https://travis-ci.org/pytest-dev/pytest/jobs/506730858#L877-L900 |
|
Thanks everyone for your comments! I'll take them on board, and open a new PR against |
JFI: you can edit the base branch (top right, with the title) of a PR - and then force-push the rebased one. |
|
Oh, awesome! I never realised that was for more than just the title, thanks 😄 |
One of the most common problems I see, and help debug, is typos in
@pytest.mark.whateverdecorators. Just recently we've had #4639 and #4814, and I've read three sets of unrelated docs in the last week that explicitly point out the spelling ofmark.parametrize(notparameterize!).The
--strictargument helps, but is clearly insufficient by default. On the other hand, it can't be an error until a major version bump. This PR emits a warning every time an unknown name is used as an attribute ofpytest.mark, and therefore closes #4826.TODO: tests, if people otherwise like the approach.