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 11 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 251 in src/_pytest/outcomes.py
Codecov / codecov/patch
src/_pytest/outcomes.py#L250-L251
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 253 in src/_pytest/outcomes.py
Codecov / codecov/patch
src/_pytest/outcomes.py#L253
Check warning on line 256 in src/_pytest/outcomes.py
Codecov / codecov/patch
src/_pytest/outcomes.py#L255-L256
Check warning on line 266 in src/_pytest/outcomes.py
Codecov / codecov/patch
src/_pytest/outcomes.py#L266
Check warning on line 270 in src/_pytest/outcomes.py
Codecov / codecov/patch
src/_pytest/outcomes.py#L270
Check warning on line 273 in src/_pytest/outcomes.py
Codecov / codecov/patch
src/_pytest/outcomes.py#L273
Check warning on line 282 in src/_pytest/outcomes.py
Codecov / codecov/patch
src/_pytest/outcomes.py#L282
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 285 in src/_pytest/outcomes.py
Codecov / codecov/patch
src/_pytest/outcomes.py#L285
Check warning on line 287 in src/_pytest/outcomes.py
Codecov / codecov/patch
src/_pytest/outcomes.py#L287
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. 😁