Skip to content

Bug: assert_produces_warning(None) not raising AssertionError with warning #38626

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Dec 22, 2020
Merged

Bug: assert_produces_warning(None) not raising AssertionError with warning #38626

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
724a2af
BUG: assert_produces_warning() with None or False doesn't raise
mzeitlin11 Dec 21, 2020
0a625f3
Remove commented line
mzeitlin11 Dec 21, 2020
2e1d58a
Honor extra warnings arg
mzeitlin11 Dec 21, 2020
aed3465
Add test for None with false raise on extra
mzeitlin11 Dec 21, 2020
c82b859
Refactor added tests
mzeitlin11 Dec 21, 2020
1938ad9
Merge remote-tracking branch 'upstream/master' into bug/assert_produc…
mzeitlin11 Dec 22, 2020
6f3c8ff
xfail failing tests
mzeitlin11 Dec 22, 2020
8193ea1
xfail parser test
mzeitlin11 Dec 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions pandas/_testing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2724,11 +2724,10 @@ class for all warnings. To check that no warning is returned,
extra_warnings = []

for actual_warning in w:
Copy link
Member

Choose a reason for hiding this comment

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

isn't this code functionally the same as existing?

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 thought if expected_warning is False or None, then extra_warnings does not get appended to because of the continue. So the check at the end for raising on extra warnings doesn't get triggered.

Copy link
Member

@ivanovmg ivanovmg Dec 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Current implementation will ignore the else clause below, which consolidates extra warnings. I see that I messed that up in one of my previous PRs. This PR reverts that error.
Probably, if some functions are extracted, that would improve the readability of the logic (maybe in a separate PR).

Copy link
Member Author

Choose a reason for hiding this comment

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

The added tests do raise on master as well, so behavior for them has changed

if not expected_warning:
continue

expected_warning = cast(Type[Warning], expected_warning)
if issubclass(actual_warning.category, expected_warning):
if expected_warning and issubclass(
actual_warning.category, expected_warning
):
Comment on lines -2727 to +2730
Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is a good catch.

saw_warning = True

if check_stacklevel and issubclass(
Expand Down
17 changes: 17 additions & 0 deletions pandas/tests/util/test_assert_produces_warning.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,3 +152,20 @@ def test_right_category_wrong_match_raises(pair_different_warnings):
with tm.assert_produces_warning(target_category, match=r"^Match this"):
warnings.warn("Do not match it", target_category)
warnings.warn("Match this", other_category)


@pytest.mark.parametrize("false_or_none", [False, None])
Copy link
Member

Choose a reason for hiding this comment

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

Good tests to prevent the issue from happening!

class TestFalseOrNoneExpectedWarning:
def test_raise_on_warning(self, false_or_none):
msg = r"Caused unexpected warning\(s\)"
with pytest.raises(AssertionError, match=msg):
with tm.assert_produces_warning(false_or_none):
f()

def test_no_raise_without_warning(self, false_or_none):
with tm.assert_produces_warning(false_or_none):
pass

def test_no_raise_with_false_raise_on_extra(self, false_or_none):
with tm.assert_produces_warning(false_or_none, raise_on_extra_warnings=False):
f()