Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Emit warning on unknown marks via decorator #4933

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelog/4826.feature.rst
@@ -0,0 +1,2 @@
A warning is now emitted when unknown marks are used as a decorator.
This is often due to a typo, which can lead to silently broken tests.
22 changes: 14 additions & 8 deletions doc/en/mark.rst
Expand Up @@ -26,14 +26,15 @@ which also serve as documentation.
:ref:`fixtures <fixtures>`.


Raising errors on unknown marks: --strict
-----------------------------------------
.. _unknown-marks:

When the ``--strict`` command-line flag is passed, any unknown marks applied
with the ``@pytest.mark.name_of_the_mark`` decorator will trigger an error.
Marks defined or added by pytest or by a plugin will not trigger an error.
Raising errors on unknown marks
-------------------------------

Marks can be registered in ``pytest.ini`` like this:
Unknown marks applied with the ``@pytest.mark.name_of_the_mark`` decorator
will always emit a warning, in order to avoid silently doing something
surprising due to mis-typed names. You can disable the warning for custom
marks by registering them in ``pytest.ini`` like this:

.. code-block:: ini

Expand All @@ -42,8 +43,13 @@ Marks can be registered in ``pytest.ini`` like this:
slow
serial

This can be used to prevent users mistyping mark names by accident. Test suites that want to enforce this
should add ``--strict`` to ``addopts``:
When the ``--strict`` command-line flag is passed, any unregistered marks
applied with the ``@pytest.mark.name_of_the_mark`` decorator will trigger an
error, including built-in marks such as ``skip`` or ``xfail``.
Zac-HD marked this conversation as resolved.
Show resolved Hide resolved

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``:
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

i like to see --strict go away as the current form is simply very lacking

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd prefer to leave that to a future PR, since I don't have time to do that too at the moment.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough! 👍


.. code-block:: ini

Expand Down
3 changes: 1 addition & 2 deletions src/_pytest/mark/__init__.py
Expand Up @@ -146,8 +146,7 @@ def pytest_collection_modifyitems(items, config):

def pytest_configure(config):
config._old_mark_config = MARK_GEN._config
if config.option.strict:
MARK_GEN._config = config
MARK_GEN._config = config

empty_parameterset = config.getini(EMPTY_PARAMETERSET_OPTION)

Expand Down
58 changes: 40 additions & 18 deletions src/_pytest/mark/structures.py
Expand Up @@ -11,6 +11,7 @@
from ..compat import MappingMixin
from ..compat import NOTSET
from _pytest.outcomes import fail
from _pytest.warning_types import UnknownMarkWarning

EMPTY_PARAMETERSET_OPTION = "empty_parameter_set_mark"

Expand Down Expand Up @@ -129,7 +130,7 @@ def _for_parametrize(cls, argnames, argvalues, func, config, function_definition
)
else:
# empty parameter set (likely computed at runtime): create a single
# parameter set with NOSET values, with the "empty parameter set" mark applied to it
# parameter set with NOTSET values, with the "empty parameter set" mark applied to it
mark = get_empty_parameterset_mark(config, argnames, func)
parameters.append(
ParameterSet(values=(NOTSET,) * len(argnames), marks=[mark], id=None)
Expand All @@ -152,7 +153,7 @@ def combined_with(self, other):
:type other: Mark
:rtype: Mark

combines by appending aargs and merging the mappings
combines by appending args and merging the mappings
"""
assert self.name == other.name
return Mark(
Expand Down Expand Up @@ -284,27 +285,48 @@ def test_function():

_config = None

# These marks are always treated as known for the warning, which is designed
Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

Choose a reason for hiding this comment

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

🤦‍♂️ They already do, actually. That'll simplify the implementation a bit...

# to mitigate typos. However they are ignored by the --strict option, which
# requires explicit registration, to match previous behaviour.
_builtin_marks = {
"filterwarnings",
"parametrize",
"skip",
"skipif",
"usefixtures",
"xfail",
}
_markers = set()

def __getattr__(self, name):
if name[0] == "_":
raise AttributeError("Marker name must NOT start with underscore")
if self._config is not None:
self._check(name)
self._update_markers(name)
if name not in self._markers:
if name not in self._builtin_marks:
warnings.warn(
"Unknown mark %r. You can register custom marks to avoid this "
"warning, without risking typos that silently break your tests. "
"See https://docs.pytest.org/en/latest/mark.html for more detail.",
UnknownMarkWarning,
)
if self._config is not None and self._config.option.strict:
fail("{!r} not a registered marker".format(name), pytrace=False)

return MarkDecorator(Mark(name, (), {}))

def _check(self, name):
try:
if name in self._markers:
return
except AttributeError:
pass
self._markers = values = set()
for line in self._config.getini("markers"):
marker = line.split(":", 1)[0]
marker = marker.rstrip()
x = marker.split("(", 1)[0]
values.add(x)
if name not in self._markers:
fail("{!r} not a registered marker".format(name), pytrace=False)
def _update_markers(self, name=None):
# We store a set of registered markers as a performance optimisation,
# but more could be added to `self._config` by other plugins at runtime.
# If we see an unknown marker, we therefore update the set and try again!
if (
self._config is not None
and name not in self._markers
and (self._config.option.strict or name not in self._builtin_marks)
):
for line in self._config.getini("markers"):
marker = line.split(":", 1)[0].rstrip().split("(", 1)[0]
self._markers.add(marker)


MARK_GEN = MarkGenerator()
Expand Down
8 changes: 8 additions & 0 deletions src/_pytest/warning_types.py
Expand Up @@ -9,6 +9,14 @@ class PytestWarning(UserWarning):
"""


class UnknownMarkWarning(PytestWarning):
Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, taking it out of this one.

"""
Bases: :class:`pytest.PytestWarning`.

Warning class for unknown attributes of the pytest.mark decorator factory.
"""


class PytestDeprecationWarning(PytestWarning, DeprecationWarning):
"""
Bases: :class:`pytest.PytestWarning`, :class:`DeprecationWarning`.
Expand Down
3 changes: 3 additions & 0 deletions tox.ini
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

This marker should be registered by pytester I think


[flake8]
max-line-length = 120
Expand Down