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 #4935

Merged
merged 4 commits into from Apr 2, 2019

Conversation

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

commented Mar 16, 2019

Being an updated version of #4933.

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. This PR emits a warning every time an unknown name is used as an attribute of pytest.mark, and therefore closes #4826.

TODO: tests, if people otherwise like the approach.

@Zac-HD

This comment has been minimized.

Copy link
Member Author

commented Mar 16, 2019

... no idea at all what the merge conflict is about - GitHub isn't saying and I can't reproduce it locally 😭

@RonnyPfannschmidt

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

it looks like the plain git merge is being confused by many of the changes, simply rebase onto the latest features branch

@RonnyPfannschmidt

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

to elaborate - i strongly suspect that your local ux merge tool is smart enough to sort those non-conflicts, but the git without it face-plants horrendously

@Zac-HD Zac-HD force-pushed the Zac-HD:warn-unknown-marks branch 2 times, most recently from 067328a to e553869 Mar 16, 2019

@codecov

This comment has been minimized.

Copy link

commented Mar 17, 2019

Codecov Report

Merging #4935 into features will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##           features    #4935      +/-   ##
============================================
+ Coverage     96.03%   96.06%   +0.03%     
============================================
  Files           114      114              
  Lines         25750    25745       -5     
  Branches       2548     2548              
============================================
+ Hits          24728    24731       +3     
+ Misses          711      704       -7     
+ Partials        311      310       -1
Impacted Files Coverage Δ
src/_pytest/warning_types.py 100% <100%> (ø) ⬆️
src/_pytest/mark/structures.py 92.39% <100%> (-0.22%) ⬇️
src/_pytest/mark/__init__.py 97.53% <100%> (-0.04%) ⬇️
testing/acceptance_test.py 98.03% <0%> (+0.39%) ⬆️
src/_pytest/doctest.py 96.41% <0%> (+2.15%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 407d74b...cab4069. Read the comment docs.

@Zac-HD Zac-HD force-pushed the Zac-HD:warn-unknown-marks branch from e553869 to bbc0de5 Mar 17, 2019

@Zac-HD Zac-HD force-pushed the Zac-HD:warn-unknown-marks branch from 2a54fef to 9121138 Mar 31, 2019

Zac-HD added some commits Mar 31, 2019

Use mark-specific warning type
So that we can ignore it in self-tests.
Show resolved Hide resolved src/_pytest/warning_types.py
Show resolved Hide resolved src/_pytest/mark/structures.py Outdated
Show resolved Hide resolved src/_pytest/mark/structures.py Outdated
Show resolved Hide resolved src/_pytest/mark/structures.py Outdated
Show resolved Hide resolved tox.ini
Show resolved Hide resolved src/_pytest/mark/structures.py

@Zac-HD Zac-HD merged commit 1c9dcf1 into pytest-dev:features Apr 2, 2019

5 checks passed

WIP Ready for review
Details
codecov/patch 100% of diff hit (target 96.03%)
Details
codecov/project 96.06% (+0.03%) compared to 407d74b
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
pytest-CI #20190402.2 succeeded
Details
@blueyed

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

No tests for the warnings though? (I wanted to check if the same mark would issue multiple warnings, which it likely does)

return MarkDecorator(Mark(name, (), {}))
# We store a set of markers as a performance optimisation - if a mark
# name is in the set we definitely know it, but a mark may be known and
# not in the set. We therefore start by updating the set!

This comment has been minimized.

Copy link
@blueyed

blueyed Apr 8, 2019

Contributor

When can it happen that it is known, but not in the set? (except for the first read of the config of course)

(I've notied that the config is re-processed again for every unknown mark, whereas previously it was "read" just once)

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Apr 9, 2019

Member

I think this covers the case where marks can be registered later, for example by pytest_configure hooks in conftests.py not at the root.

This comment has been minimized.

Copy link
@Zac-HD

Zac-HD Apr 10, 2019

Author Member

Yep, pytest_configure can run later than I had expected.

The re-processing isn't free, but the number of unknown marks should be relatively small and it's necessary for correctness.

# example lines: "skipif(condition): skip the given test if..."
# or "hypothesis: tests which use Hypothesis", so to get the
# marker name we we split on both `:` and `(`.
marker = line.split(":")[0].split("(")[0].strip()

This comment has been minimized.

Copy link
@blueyed

blueyed Apr 8, 2019

Contributor

This used maxsplit=1 before (which makes sense to keep).

@nicoddemus

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

No tests for the warnings though? (I wanted to check if the same mark would issue multiple warnings, which it likely does)

Oh my thanks for catching that.

@Zac-HD do you have the time to add one or more tests for this? Otherwise I can tackle it in #5023.

@Zac-HD

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2019

@nicoddemus - unfortunately I now have an unexpected work trip next week, so I really don't have time to do anything except prepare my stuff for PyCon 😕

@nicoddemus

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

Sure thing, no worries, thanks! Have a great PyCon! 👍

@Zac-HD Zac-HD deleted the Zac-HD:warn-unknown-marks branch Apr 24, 2019

seanh added a commit to hypothesis/h that referenced this pull request May 13, 2019

Remove @pytest.mark.fuzz
This was an unregistered pytest mark, and pytest now emits a warning,
which causes our tests to fail, when an unregistered mark is used:

pytest-dev/pytest#4935

Rather than registering the mark simply delete all uses of it. All tests
that're defined using hypothesis automatically have the
`@pytest.mark.hypothesis` mark applied to them so it's not necessary to
try to remember to manually apply a custom mark to every test that uses
hypothesis:

https://hypothesis.readthedocs.io/en/latest/details.html#the-hypothesis-pytest-plugin
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.