Skip to content

Raise a meaningful error message in @image_comparison decorator #5518

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Raise a meaningful error message in @image_comparison decorator #5518

wants to merge 2 commits into from

Conversation

maxalbert
Copy link
Contributor

This PR makes the use of the @image_comparison decorator slightly more user-friendly in projects outside matplotlib. (Previously, an AssertionError with a fairly cryptic error message would be raised if a test that uses the @image_comparison decorator is not contained in a submodule named tests.)

Btw, is there any specific reason this assertion exists? Why not simply allow the tests to live in any directory?

@maxalbert
Raise a meaningful error message to make the use of the @image_compar…
a3875dd 
…ison decorator more user-friendly for projects other than matplotlib.
@maxalbert
Change exception type back to AssertionError (in a somewhat shot-in-t…
8683f54 
…he-dark attempt to fix the Travis build with numpy 1.6).
@tacaswell
Copy link
Member

That particular issue is a new intermittent png issue that we are still trying to track down.

@mdboom Thoughts on just removing this?

@tacaswell tacaswell added this to the proposed next point release (2.1) milestone Nov 19, 2015
@mdboom
Copy link
Member

mdboom commented Nov 19, 2015

That particular issue is a new intermittent png issue that we are still trying to track down.

I don't think this has to do with the PNG issue.

It has to do with how the path to the test file is automatically determined (which is used to get the name for the output directory). This was always a pain because it would be different in different contexts (matplotlib.tests() vs. nosetests at the commandline etc.)

It can probably be removed because it was just a sanity check. I don't think there's any good reason to force the behavior.

@mdboom
Copy link
Member

mdboom commented Nov 19, 2015

Oh -- I see what you meant by the PNG error now.

I vote for just removing the assert altogether and not insisting on a special name for the root of the tests.

@maxalbert
Copy link
Contributor Author

Thanks for the quick replies! To be honest, now I'm slightly confused which particular issues both of you were referring to and what the desired behaviour is. ;-) Since it sounds like it would be good to remove the assert I presume this PR is obsolete, so feel free to close it and simply apply the desired changes yourselves. (Or let me know how you'd like the PR to be reworked and I can try to give it a shot.)

@tacaswell
Copy link
Member

Could you take a shot at removing it?

On Fri, Nov 20, 2015, 02:42 maxalbert notifications@github.com wrote:

Thanks for the quick replies! To be honest, now I'm slightly confused
which particular issues both of you were referring to and what the desired
behaviour is. ;-) Since it sounds like it would be good to remove the
assert I presume this PR is obsolete, so feel free to close it and simply
apply the desired changes yourselves. (Or let me know how you'd like the PR
to be reworked and I can try to give it a shot.)

—
Reply to this email directly or view it on GitHub
#5518 (comment)
.

@maxalbert
Copy link
Contributor Author

Just to make sure I understand correctly: by "removing it" you mean "removing the requirement that the test needs to be defined in a submodule named tests"? Yes, I can have a go. (Although I may not get around to it before next week in case this change has further ramifications, as indicated by the strange failure of the first Travis build for this PR when I removed the AssertionError.)

@jenshnielsen
Copy link
Member

Yes get rid of that requirement. The error in the first commit from Travis is just a random fluke and not related to your changes.

@maxalbert maxalbert mentioned this pull request Jan 13, 2016
Merged
@QuLogic
Copy link
Member

QuLogic commented Jan 13, 2016

Replaced by #5842.

@QuLogic QuLogic closed this Jan 13, 2016
@QuLogic QuLogic modified the milestones: unassigned, proposed next point release (2.1) Jan 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
status: duplicate
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@maxalbert @tacaswell @mdboom @jenshnielsen @QuLogic