Skip to content
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

BUG, TST: fix-_check_ticks_props #34768

Merged
merged 7 commits into from
Jun 16, 2020
Merged

BUG, TST: fix-_check_ticks_props #34768

merged 7 commits into from
Jun 16, 2020

Conversation

MarcoGorelli
Copy link
Member

@MarcoGorelli MarcoGorelli commented Jun 14, 2020 •
edited
Loading

Here's something I noticed while working on #34334 : self._check_ticks_props(ax, ylabelsize=0) always passes!

This is because of

if ylabelsize

instead of

if ylabelsize is not None

being used.

The test I've added fails on master:

(pandas-dev) marco@marco-Predator-PH315-52:~/pandas-dev$ pytest pandas/tests/plotting/test_frame.py::TestDataFramePlots::test_plot_with_rot
============================================================== test session starts ==============================================================
platform linux -- Python 3.8.2, pytest-5.4.1, py-1.8.1, pluggy-0.13.1
rootdir: /home/marco/pandas-dev, inifile: setup.cfg
plugins: cov-2.8.1, xdist-1.31.0, asyncio-0.10.0, hypothesis-5.8.0, forked-1.1.2
collected 1 item                                                                                                                                

pandas/tests/plotting/test_frame.py .                                                                                                     [100%]

=============================================================== 1 passed in 0.33s ===============================================================  
(pandas-dev) marco@marco-Predator-PH315-52:~/pandas-dev$ git checkout upstream/master -- pandas/tests/plotting/common.py
(pandas-dev) marco@marco-Predator-PH315-52:~/pandas-dev$ pytest pandas/tests/plotting/test_frame.py::TestDataFramePlots::test_plot_with_rot
============================================================== test session starts ==============================================================
platform linux -- Python 3.8.2, pytest-5.4.1, py-1.8.1, pluggy-0.13.1
rootdir: /home/marco/pandas-dev, inifile: setup.cfg
plugins: cov-2.8.1, xdist-1.31.0, asyncio-0.10.0, hypothesis-5.8.0, forked-1.1.2
collected 1 item                                                                                                                                

pandas/tests/plotting/test_frame.py F                                                                                                     [100%]

=================================================================== FAILURES ====================================================================
_____________________________________________________ TestDataFramePlots.test_plot_with_rot _____________________________________________________

self = <pandas.tests.plotting.test_frame.TestDataFramePlots object at 0x7f8c7578f3a0>

    def test_plot_with_rot(self):
        # GH 34768
        df = pd.DataFrame({"b": [0, 1, 0], "a": [1, 2, 3]})
        ax = _check_plot_works(df.plot, rot=30)
        ax.yaxis.set_tick_params(rotation=30)
        msg = "expected 0.00000 but got "
        with pytest.raises(AssertionError, match=msg):
>           self._check_ticks_props(ax, xrot=0)
E           Failed: DID NOT RAISE <class 'AssertionError'>

pandas/tests/plotting/test_frame.py:3360: Failed
============================================================ short test summary info ============================================================
FAILED pandas/tests/plotting/test_frame.py::TestDataFramePlots::test_plot_with_rot - Failed: DID NOT RAISE <class 'AssertionError'>
=============================================================== 1 failed in 0.46s ===============================================================

@MarcoGorelli MarcoGorelli changed the title fix-_check_ticks_props BUG, TST: fix-_check_ticks_props Jun 14, 2020
@jreback jreback added Testing pandas testing functions or related to the test suite Visualization plotting labels Jun 14, 2020
@MarcoGorelli MarcoGorelli requested a review from WillAyd June 14, 2020 18:06
WillAyd
WillAyd requested changes Jun 15, 2020
View reviewed changes
Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Just a comment on test location but lgtm otherwise

pandas/tests/plotting/test_frame.py Outdated
@@ -3350,6 +3352,21 @@ def test_colors_of_columns_with_same_name(self):
for legend, line in zip(result.get_legend().legendHandles, result.lines):
assert legend.get_color() == line.get_color()

def test__check_ticks_props(self):
Copy link
Member

Choose a reason for hiding this comment

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

Can you put this test in a separate module? test_common.py should be fine

WillAyd
WillAyd reviewed Jun 16, 2020
View reviewed changes
pandas/tests/plotting/test_frame.py Outdated
@@ -48,6 +48,7 @@ def _assert_xtickslabels_visibility(self, axes, expected):
for ax, exp in zip(axes, expected):
self._check_visible(ax.get_xticklabels(), visible=exp)

@pytest.mark.xfail(reason="Waiting for PR 34334")
Copy link
Member

Choose a reason for hiding this comment

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

Just for my understanding this is still running on CI right? Not immediately clear why these need to be xfailed now but I assume this pre-cursor to the mentioned PR highlights an issue with how these tests are written?

Copy link
Member Author

@MarcoGorelli MarcoGorelli Jun 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The problem with this test is the line

self._check_ticks_props(ax, xrot=0)

. On master, self._check_ticks_props(ax, xrot=0) always passes (regardless of what xrot actually is in the plot). And so even though xrot is 30 in this plot, the test still passes.

However, if we change self._check_ticks_props(ax, xrot=0) so that it actually catches errors, then this test will fail, because the xticklabels are being unnecessarily rotated.

PR #34334 would remove the unnecessary rotation, and then this test would go back to passing (as expected).

@WillAyd
Copy link
Member

WillAyd commented Jun 16, 2020

@TomAugspurger care to look?

@jreback jreback added this to the 1.1 milestone Jun 16, 2020
@TomAugspurger TomAugspurger merged commit df6f668 into pandas-dev:master Jun 16, 2020
@TomAugspurger
Copy link
Contributor

Thanks.

@MarcoGorelli MarcoGorelli deleted the fix-check-rot branch June 16, 2020 19:34
@MarcoGorelli MarcoGorelli mentioned this pull request Dec 10, 2020
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WillAyd WillAyd WillAyd approved these changes

Assignees
No one assigned
Labels
Testing pandas testing functions or related to the test suite Visualization plotting
Projects
None yet
Milestone
1.1
Development

Successfully merging this pull request may close these issues.
4 participants
@MarcoGorelli @WillAyd @TomAugspurger @jreback