Movatterモバイル変換


[0]ホーム

URL:


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

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

@tacaswell
Copy link
Member

@tacaswelltacaswell commentedMar 6, 2020
edited
Loading

PR Summary

If you stackedpytest.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 isFlake 8 compliant

@tacaswelltacaswell added this to thev3.3.0 milestoneMar 6, 2020
If you stacked `pytest.mark.parametrize` with check_figures_equalevery set of parameters would write to the same file.  This makespost-hoc debugging hard and causes intermittent CI failures.
They are preceded by a `*args` in the signature of wrapper.
@tacaswelltacaswellforce-pushed thetst_better_compare_names branch from765e08e to3b57fbaCompareMarch 6, 2020 14:28
Copy link
Contributor

@anntzeranntzer 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

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

@tacaswell
Copy link
MemberAuthor

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 usingthe 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 inPurePosixPath \PureWindowsPath.

@tacaswell
Copy link
MemberAuthor

The failure was

_________________________ test_loglog_nonpos[png-True] _________________________[gw0] linux -- Python 3.8.2 /opt/hostedtoolcache/Python/3.8.2/x64/bin/pythonexpected = '/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
MemberAuthor

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

Indeed thecheck_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 commentedMar 16, 2020
edited
Loading

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

@QuLogicQuLogic modified the milestones:v3.3.0,v3.2.1Mar 16, 2020
@QuLogicQuLogic merged commitb4a414f intomatplotlib:masterMar 16, 2020
@lumberbot-app
Copy link

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 tosuggest an improvement.

QuLogic added a commit to QuLogic/matplotlib that referenced this pull requestMar 16, 2020
…_namesTST: use pytest name in naming files for check_figures_equal
QuLogic added a commit to QuLogic/matplotlib that referenced this pull requestMar 16, 2020
…_namesTST: use pytest name in naming files for check_figures_equal
@QuLogic
Copy link
Member

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

tacaswell reacted with thumbs up emoji

@tacaswelltacaswell deleted the tst_better_compare_names branchMarch 16, 2020 23:04
@QuLogic
Copy link
Member

QuLogic commentedMar 17, 2020
edited
Loading

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

______ 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...

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@QuLogicQuLogicQuLogic left review comments

@anntzeranntzeranntzer approved these changes

@timhoffmtimhoffmtimhoffm approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

v3.2.1

Development

Successfully merging this pull request may close these issues.

4 participants

@tacaswell@QuLogic@timhoffm@anntzer

[8]ページ先頭

©2009-2025 Movatter.jp