-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Test simplifications. #9497
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
Test simplifications. #9497
Conversation
lib/matplotlib/tests/test_axes.py
Outdated
"add_axes": | ||
lambda fig: fig.add_axes([.1, .1, .8, .8]), | ||
}[request.param] | ||
return fig, make_ax(fig) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see this code prettier and actually it only partly checks what the functions return.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point is to not have to add code to handle a case that should never happen (request.param being an invalid value -- it is controlled by the fixture params) while keeping clear the fact that an error (a KeyError) will be thrown if an invalid value ever comes in (... through some severely messed up bug in pytest).
If others also disagree (happy to hear more opinions), I'll revert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure the use of lambda
s stored in a dict
is improving readability here. Removing the else
on the if is fine with me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the offending part.
# try getting the axes given an == (not is) polar projection | ||
ax_via_gca = plt.gca(projection=prj3) | ||
assert ax_via_gca is ax | ||
plt.close() | ||
|
||
# testing axes creation with subplot | ||
ax = plt.subplot(121, projection=prj) | ||
assert type(ax) == maxes._subplots._subplot_classes[PolarAxes], \ | ||
'Expected a PolarAxesSubplot, got %s' % type(ax) | ||
assert type(ax) == maxes._subplots._subplot_classes[PolarAxes] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be isinstance
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the point is to have an exact type check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, my memory on isinstance
was fuzzy at that moment.
💯 on the idea behind this patch, just some minor things to work out. |
- `assert_produces_warning` is now unused and has been replaced in the test suite by `pytest.warns`. - Replace `assert x, y` by `assert x` when the information in `y` is clearly redundant with what pytest's assert rewrite would produce. (There are a few cases where the message actually provides more information, these are left as is.)
All issues addressed, I think. |
assert_produces_warning
is now unused and has been replaced in thetest suite by
pytest.warns
.Replace
assert x, y
byassert x
when the information iny
isclearly redundant with what pytest's assert rewrite would produce.
(There are a few cases where the message actually provides more
information, these are left as is.)
PR Summary
PR Checklist