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 warnings on use of unknown marks #4826

Closed
Zac-HD opened this issue Feb 25, 2019 · 17 comments

Comments

Projects
None yet
5 participants
@Zac-HD
Copy link
Member

commented Feb 25, 2019

One of the most common problems I see, and help debug, is typos in @pytest.mark.whatever decorators. 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 of mark.parametrize (not parameterize!).

The --strict argument helps, but is clearly insufficient by default. On the other hand, it can't be an error until a major version bump.

Happily, there is a standard middle ground: warnings! I therefore propose to emit a warning every time an unknown name is used as an attribute of pytest.mark. The message should include how to register the mark, why unregistered marks are problematic, and note that this warning may become an error in a future version of pytest.

Unless there are major objections to this plan, I'll try for an implementation over the weekend.

@blueyed

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

@Zac-HD
Sounds good, looking forward to a PR.

@RonnyPfannschmidt

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

short opinion on this one, i believe we should migrate to warnings and deprecate --strict

if necessary i'm going to elaborate deeper when i'm back to active

@Zac-HD

This comment has been minimized.

Copy link
Member Author

commented Mar 6, 2019

Here's a quick prototype: master...Zac-HD:warn-unknown-marks

Roughly what you had in mind? Any feedback would be great!

@RonnyPfannschmidt

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

@Zac-HD as far as i can tell its broken due to adding more caching behaviour than warranted

@Zac-HD

This comment has been minimized.

Copy link
Member Author

commented Mar 6, 2019

Ah, right - I had assumed that the config object was much more static that it really was. Now fixed, I think, so another quick review would be great 😄

@blueyed

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

@Zac-HD
A (draft) PR would be better, since you could comment on the code then.

As for deprecating --strict: I think it could be kepts still, and be used for other things in the future also. It is not --fail-on-unknown-marks after all.

@Zac-HD

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2019

Closed by #4935.

@1313e

This comment has been minimized.

Copy link

commented May 23, 2019

In v4.5.0, this now emits warnings about 'unknown marks' that are perfectly valid and known.
For example, using the pytest_pep8 plugin will emit that warning about pytest.mark.pep8, even though it is known and can be accessed perfectly fine.

@nicoddemus

This comment has been minimized.

Copy link
Member

commented May 23, 2019

Hi @1313e, thanks for writing.

You mean pytest_pep8 does register the pep8 mark and pytest still complains about it being unknown? Then that's certainly a bug.

@1313e

This comment has been minimized.

Copy link

commented May 23, 2019

Hi @1313e, thanks for writing.

You mean pytest_pep8 does register the pep8 mark and pytest still complains about it being unknown? Then that's certainly a bug.

Correct.

@nicoddemus

This comment has been minimized.

Copy link
Member

commented May 23, 2019

@1313e

You mean pytest_pep8 does register the pep8 mark and pytest still complains about it being unknown?

Correct.

Took a quick look and that does not seem to be the case:

https://bitbucket.org/pytest-dev/pytest-pep8/src/44ddb3739f0f892fc0029d5d809496ec5ef24f28/pytest_pep8.py#lines-12:21

The pep8 mark should be registered like shown here.

@Zac-HD

This comment has been minimized.

Copy link
Member Author

commented May 23, 2019

The pep8 mark should be registered like shown here.

Note that this is important to get the correct command-line help, as well as to avoid the new warning!

@1313e

This comment has been minimized.

Copy link

commented May 23, 2019

That might be, but it still raises the warning.
So, apparently, it acts as if it is not registered, while it does work perfectly fine.

@RonnyPfannschmidt

This comment has been minimized.

Copy link
Member

commented May 23, 2019

the point of the warning is to tell you there is something missing without actually breaking the world
so its not perfectly fine

@1313e

This comment has been minimized.

Copy link

commented May 23, 2019

the point of the warning is to tell you there is something missing without actually breaking the world
so its not perfectly fine

Uhm, thanks for making my point I guess?

@blueyed

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

@1313e
The problem is that the mark is not registered apparently, but only used. This emits a warning, since it should get registered.

@nicoddemus

This comment has been minimized.

Copy link
Member

commented May 23, 2019

@1313e

Just to complement the other answers:

  1. A plugin can use a mark (like pytest-pep8 does) to drive some behavior;
  2. A plugin should register the mark during their initialization; this shows the mark with pytest --help and lets pytest know the mark exists.
  3. A user can use a mark in a test with the @pytest.mark decorator.

What is happening that pytest-pep8 uses the mark (1), users can mark tests with that mark (3), but the mark itself is not registered by the plugin (2).

The warning is emitted because typos in marks are a common source of mistakes: if a user mistypes ppe8, then the plugin won't work as expected, leading to confusing and error reports. The new warning prevents that.

This causes a small pain now, but will prevent a lot of headaches in the future; plugins will now register their marks from the get-go, and mistypes will be immediately obvious.

Reports about the unknown marks in the respective plugin bug trackers are welcome, so plugin authors can be made aware of the missing registration. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.