Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changed importError to ModuleNotFoundError #12220
Changed importError to ModuleNotFoundError #12220
Changes from 10 commits
3a26211
3f80fd7
fd288e1
1fe14e0
543262c
1456b71
9a9643d
3ca5036
7de869d
3ca329b
f14d78d
b59336a
76fd0e5
a55abeb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 250 in src/_pytest/outcomes.py
Codecov / codecov/patch
src/_pytest/outcomes.py#L249-L250
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused by the Codecov annotations here. It looks to me like this (and the others) should be covered by tests just fine? Are the added tests not running or something? Or is codecov broken? Or am I just missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, but this is definitely being covered by tests (and normal tests, not through
pytester
either). I even tested this locally by adding a hardassert 0
in this line to ensure it is hit by the tests, and it is (as expected).Not sure what's going on.
Check warning on line 252 in src/_pytest/outcomes.py
Codecov / codecov/patch
src/_pytest/outcomes.py#L252
Check warning on line 255 in src/_pytest/outcomes.py
Codecov / codecov/patch
src/_pytest/outcomes.py#L254-L255
Check warning on line 265 in src/_pytest/outcomes.py
Codecov / codecov/patch
src/_pytest/outcomes.py#L265
Check warning on line 269 in src/_pytest/outcomes.py
Codecov / codecov/patch
src/_pytest/outcomes.py#L269
Check warning on line 272 in src/_pytest/outcomes.py
Codecov / codecov/patch
src/_pytest/outcomes.py#L272
Check warning on line 281 in src/_pytest/outcomes.py
Codecov / codecov/patch
src/_pytest/outcomes.py#L281
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not call
warning.warn(...)
here directly, given thatwarning = ...
is only set from here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because at this point we are inside the
with warnings.catch_warnings():
block, which would catch our own warning.Check warning on line 284 in src/_pytest/outcomes.py
Codecov / codecov/patch
src/_pytest/outcomes.py#L284
Check warning on line 286 in src/_pytest/outcomes.py
Codecov / codecov/patch
src/_pytest/outcomes.py#L286
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, couldn't this be simplified by moving the
raise Skipped(...)
to directly after the warning?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per #12220 (comment), we need to raise this after the warning. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love this! Never seen it before, I've always done:
for this kind of thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All credits go to https://www.cosmicpython.com, seen this pattern there and have been using it ever since. 😁