Skip to content

TST: use pytest name in naming files for check_figures_equal #16693

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

Merged
merged 4 commits into from
Mar 16, 2020

Conversation

tacaswell
Copy link
Member

@tacaswell tacaswell commented Mar 6, 2020

PR Summary

If you stacked pytest.mark.parametrize with check_figures_equal
every pass through would write to the same files. This uses the request fixture to extract the name.

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant

@tacaswell tacaswell added this to the v3.3.0 milestone Mar 6, 2020
If you stacked `pytest.mark.parametrize` with check_figures_equal
every set of parameters would write to the same file.  This makes
post-hoc debugging hard and causes intermittent CI failures.
They are preceded by a `*args` in the signature of wrapper.
@tacaswell tacaswell force-pushed the tst_better_compare_names branch from 765e08e to 3b57fba Compare March 6, 2020 14:28
Copy link
Contributor

@anntzer anntzer left a comment

Choose a reason for hiding this comment

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

postci

@QuLogic
Copy link
Member

QuLogic commented Mar 14, 2020

This seems to fail a lot; can we get it in 3.2.1 too?

@tacaswell
Copy link
Member Author

Is there an existing "make this a safe name" function someplace in the standard library or do we have to roll our own here?

This is probably to stringent but this is better than blindly using
the test name from pytest.
@timhoffm
Copy link
Member

Is there an existing "make this a safe name" function someplace in the standard library or do we have to roll our own here?

I'm not aware of anything like that. If anything I'd expect that in pathlib, but it is OS-depended, so would need to be handled in PurePosixPath \ PureWindowsPath.

@tacaswell
Copy link
Member Author

The failure was

_________________________ test_loglog_nonpos[png-True] _________________________
[gw0] linux -- Python 3.8.2 /opt/hostedtoolcache/Python/3.8.2/x64/bin/python

expected = '/home/vsts/work/1/s/result_images/test_axes/test_loglog_nonpos-expected.png'
actual = '/home/vsts/work/1/s/result_images/test_axes/test_loglog_nonpos.png'
tol = 0, in_decorator = True

This makes me think that we should be doing something similar in compare_images...

@tacaswell
Copy link
Member Author

abc9445 adds the comment from @QuLogic

            # This is hacked on via the mpl_image_comparison_parameters fixture
            # so that we don't need to modify the function's real signature for
            # any parametrization. Modifying the signature is very very tricky
            # and likely to confuse pytest.

which is definitely true! I think the tools we can use to do this are better now (3 years later).

Might be worth re-visiting that decorator to see if we can make in simpler (and stop using the in-direct fixtur paramterization that pytest tried to deprecate).

@QuLogic
Copy link
Member

QuLogic commented Mar 16, 2020

Indeed the check_figures_equal decorator does things in a much simpler way, without indirection. On the other hand, the backport of #16770 in #16791 seems to have failed in some way due to that decorator.

@QuLogic
Copy link
Member

QuLogic commented Mar 16, 2020

With #15199 backported, I think we should be able to backport this as well without conflicts.

@QuLogic QuLogic modified the milestones: v3.3.0, v3.2.1 Mar 16, 2020
@QuLogic QuLogic merged commit b4a414f into matplotlib:master Mar 16, 2020
@lumberbot-app
Copy link

lumberbot-app bot commented Mar 16, 2020

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
$ git checkout v3.2.x
$ git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
$ git cherry-pick -m1 b4a414fd5abf1b3b930bf3319ed9d1802db47be6
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
$ git commit -am 'Backport PR #16693: TST: use pytest name in naming files for check_figures_equal'
  1. Push to a named branch :
git push YOURFORK v3.2.x:auto-backport-of-pr-16693-on-v3.2.x
  1. Create a PR against branch v3.2.x, I would have named this PR:

"Backport PR #16693 on branch v3.2.x"

And apply the correct labels and milestones.

Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon!

If these instruction are inaccurate, feel free to suggest an improvement.

QuLogic added a commit to QuLogic/matplotlib that referenced this pull request Mar 16, 2020
…_names

TST: use pytest name in naming files for check_figures_equal
QuLogic added a commit to QuLogic/matplotlib that referenced this pull request Mar 16, 2020
…_names

TST: use pytest name in naming files for check_figures_equal
@QuLogic
Copy link
Member

QuLogic commented Mar 16, 2020

So we needed #15589 to avoid conflicts; it seems pretty non-controversial, so I included it in the backport above.

@tacaswell tacaswell deleted the tst_better_compare_names branch March 16, 2020 23:04
@QuLogic
Copy link
Member

QuLogic commented Mar 17, 2020

This seems to have crossed with #16707, which has a test that takes request:

______ ERROR collecting lib/matplotlib/tests/test_marker.py _____
lib/matplotlib/tests/test_marker.py:105: in <module>
    @check_figures_equal(tol=1.45)
lib/matplotlib/testing/decorators.py:419: in decorator
    inspect.Parameter("request", KEYWORD_ONLY),
/var/container/conda/envs/mpl37/lib/python3.7/inspect.py:2856: in replace
    return_annotation=return_annotation)
/var/container/conda/envs/mpl37/lib/python3.7/inspect.py:2795: in __init__
    raise ValueError(msg)
E   ValueError: duplicate parameter name: 'request'

so I guess there's a reason I wrote that indirection comment...

QuLogic added a commit that referenced this pull request Mar 17, 2020
Backport #15589 and #16693, fixes for check_figures_equal
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants