Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

MNT/TST: generalize check_figures_equal to work with pytest.marks #15199

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 5 commits into from
Oct 4, 2019
Merged

MNT/TST: generalize check_figures_equal to work with pytest.marks #15199

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c0462a5
MNT/TST: generalize check_figures_equal to work with pytest.marks
tacaswell Sep 6, 2019
f082265
MNT: don't import from private pytest module
tacaswell Sep 7, 2019
922b57f
MNT: fix spelling in test name
tacaswell Sep 7, 2019
cd4c42b
STY: condense code a bit
tacaswell Oct 4, 2019
e10850e
STY: condense code some more
tacaswell Oct 4, 2019
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
65 changes: 36 additions & 29 deletions 65 lib/matplotlib/testing/decorators.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,35 +387,42 @@ def decorator(func):

_, result_dir = _image_directories(func)

if len(inspect.signature(func).parameters) == 2:
# Free-standing function.
@pytest.mark.parametrize("ext", extensions)
def wrapper(ext):
fig_test = plt.figure("test")
fig_ref = plt.figure("reference")
func(fig_test, fig_ref)
test_image_path = result_dir / (func.__name__ + "." + ext)
ref_image_path = (
result_dir / (func.__name__ + "-expected." + ext))
fig_test.savefig(test_image_path)
fig_ref.savefig(ref_image_path)
_raise_on_image_difference(
ref_image_path, test_image_path, tol=tol)

elif len(inspect.signature(func).parameters) == 3:
# Method.
@pytest.mark.parametrize("ext", extensions)
def wrapper(self, ext):
fig_test = plt.figure("test")
fig_ref = plt.figure("reference")
func(self, fig_test, fig_ref)
test_image_path = result_dir / (func.__name__ + "." + ext)
ref_image_path = (
result_dir / (func.__name__ + "-expected." + ext))
fig_test.savefig(test_image_path)
fig_ref.savefig(ref_image_path)
_raise_on_image_difference(
ref_image_path, test_image_path, tol=tol)
@pytest.mark.parametrize("ext", extensions)
def wrapper(*args, ext, **kwargs):
fig_test = plt.figure("test")
fig_ref = plt.figure("reference")
func(*args, fig_test=fig_test, fig_ref=fig_ref, **kwargs)
test_image_path = result_dir / (func.__name__ + "." + ext)
ref_image_path = result_dir / (
func.__name__ + "-expected." + ext
)
fig_test.savefig(test_image_path)
fig_ref.savefig(ref_image_path)
_raise_on_image_difference(
ref_image_path, test_image_path, tol=tol
)

sig = inspect.signature(func)
new_sig = sig.replace(
parameters=(
[
Copy link
Member

Choose a reason for hiding this comment

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

As a side note, I find your code formatting style extremely hard to read.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree this just looks like the worst of black, things like

wrapper.pytestmark = get_unpacked_marks(
    wrapper
) + get_unpacked_marks(func)

just completely obfuscate the structure of the code -- compare with

wrapper.pytestmark = (get_unpacked_marks(wrapper)
                      + get_unpacked_marks(func))

(or with an additional newline after the opening parenthesis).

Copy link
Member Author

Choose a reason for hiding this comment

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

This is indeed black's doing.

Ironically, the biggest selling point of black for me is avoiding exactly these conversations.

Copy link
Contributor

Choose a reason for hiding this comment

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

The fact that it's automated doesn't absolve it from completely violating basic esthetics.

Copy link
Contributor

Choose a reason for hiding this comment

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

fwiw @NelleV and I have been trying to come up with a better automated scheme and it's certainly not trivial... Nevertheless, since afaict matplotlib hasn't added black as a dependency, it's a bit weird to add badly black-formatted code...

Copy link
Contributor

Choose a reason for hiding this comment

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

(And @tacaswell it looks like you've set it to 71c wide? Which is unconventional to say the least...)

Copy link
Contributor

Choose a reason for hiding this comment

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

(I personally just don't buy into the idea that code can be properly formatted without understanding the underlying semantics.)

Copy link
Member

Choose a reason for hiding this comment

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

(Juan is considering doing constrained reinforcement learning to learn the rules…)

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 I set it to 80c (to keep our pyflakes happy). Black does a slightly better job if you give it a bit more width to work with.

param
for param in sig.parameters.values()
if param.name not in {"fig_test", "fig_ref"}
]
+ [
inspect.Parameter(
"ext", inspect.Parameter.POSITIONAL_OR_KEYWORD
)
]
)
)
wrapper.__signature__ = new_sig

# reach a bit into pytest internals to hoist the marks from
# our wrapped function
new_marks = getattr(func, 'pytestmark', []) + wrapper.pytestmark
wrapper.pytestmark = new_marks

return wrapper

Expand Down
16 changes: 14 additions & 2 deletions 16 lib/matplotlib/tests/test_testing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,19 @@
import warnings
import pytest
from matplotlib.testing.decorators import check_figures_equal

@pytest.mark.xfail(strict=True,
reason="testing that warnings fail tests")

@pytest.mark.xfail(
strict=True, reason="testing that warnings fail tests"
)
def test_warn_to_fail():
warnings.warn("This should fail the test")


@pytest.mark.parametrize("a", [1])
@check_figures_equal(extensions=["png"])
@pytest.mark.parametrize("b", [1])
def test_paramatirize_with_check_figure_equal(
tacaswell marked this conversation as resolved.
Show resolved Hide resolved
a, fig_ref, b, fig_test
):
assert a == b
Morty Proxy This is a proxified and sanitized view of the page, visit original site.