Add test for _num_to_string method used in __call__ of LogFormatter#8598
Add test for _num_to_string method used in __call__ of LogFormatter#8598phobson merged 8 commits intomatplotlib:masterfrom
Conversation
| ax.xaxis.set_major_formatter(lf) | ||
|
|
||
| assert str(lf(val)) is not None | ||
|
|
There was a problem hiding this comment.
Personally, I would follow the testing style used below.
There was a problem hiding this comment.
Didn't realize that there was an option to create a fake axis without having a full plot set up.
lib/matplotlib/tests/test_ticker.py
Outdated
| # test _num_to_string method used in __call__ | ||
| temp_lf = mticker.LogFormatter() | ||
| temp_lf.axis = FakeAxis() | ||
| assert str(temp_lf(10)) is not None |
There was a problem hiding this comment.
Can we do a bit better and check that 1, 10, 100, 1000 are correctly rendered (not None is the example I used in the issue but is a pretty blunt test :) )
lib/matplotlib/tests/test_ticker.py
Outdated
| # test _num_to_string method used in __call__ | ||
| temp_lf = mticker.LogFormatter() | ||
| temp_lf.axis = FakeAxis() | ||
| assert str(temp_lf(val)) is not None |
There was a problem hiding this comment.
this is still testing that result is not None (and str(None) is not None). Can you check that these are equal to the expected value?
There was a problem hiding this comment.
Sorry missed that in earlier commit. Now done.
lib/matplotlib/tests/test_ticker.py
Outdated
| call_data = [ | ||
| 1, 10, 100, 1000 | ||
| ] | ||
|
|
There was a problem hiding this comment.
I don't think this should be here. If you want to make a full-on fixture that is used in many tests, use pytest to mark it as such. Otherwise, just define this within the test that uses it.
lib/matplotlib/tests/test_ticker.py
Outdated
| ax.set_xlim(0.5, 0.9) | ||
| self._sub_labels(ax.xaxis, subs=np.arange(2, 10, dtype=int)) | ||
|
|
||
| @pytest.mark.parametrize('val', call_data) |
There was a problem hiding this comment.
If you're not going to provide multiple values for val, I wouldn't bother parametrizing the test. Just define val/call_data inside the test.
There was a problem hiding this comment.
The list does provide multiple values to val.
phobson
left a comment
phobson
left a comment
There was a problem hiding this comment.
@patniharshit This is looks. Note that I was confused the last time I reviewed this and your parametrization was fine, but the values didn't need to be defined outside of the decorator.
Sorry for being so unclear and thanks for the PR!
5 participants
PR Summary
Closes #8597
Write tests for LogFormatter to explicitly test the return value of
__call__.PR Checklist