Skip to content

MAINT: Updated tick and category test formatting #7993

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
Merged
merged 1 commit into from
Feb 2, 2017
Merged

MAINT: Updated tick and category test formatting #7993

merged 1 commit into from
Feb 2, 2017

Conversation

madphysicist
Copy link
Contributor

@madphysicist madphysicist commented Jan 30, 2017 •
edited
Loading

Rearranged tests in lib/matplotlib/tests/test_ticker.py and made a couple of very minor changes to lib/matplotlib/tests/test_category.py. Based on comment by @story645 in PRs #7482 and #7965.

This PR should probably be merged before either of the others so I can rebase them onto it and clean things up.

This was referenced Jan 30, 2017
Closed
Merged
story645
story645 reviewed Jan 30, 2017
View reviewed changes
lib/matplotlib/tests/test_ticker.py Outdated
assert formatter.format_pct(x, display_range) == expected


class TestEndFormatter(object):
Copy link
Member

Choose a reason for hiding this comment

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

typo: EngFormatter

story645
story645 reviewed Jan 30, 2017
View reviewed changes
lib/matplotlib/tests/test_category.py Outdated
np.array(['1', '11', '3']),
[b'1', b'11', b'3'],
np.array([b'1', b'11', b'3'])],
]
Copy link
Member

@story645 story645 Jan 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

apparently an extra ] here messing up travis?

@story645
Copy link
Member

😄 thanks!

@madphysicist
Copy link
Contributor Author

Thanks for looking through the PRs. I fixed up the issues you found.

@madphysicist
Copy link
Contributor Author

I am pretty sure that this PR is ready to go. The failing tests appear to be caused by an external source.

@madphysicist madphysicist force-pushed the test-fixture-fixup branch 3 times, most recently from 6d2c93f to 37592a7 Compare January 30, 2017 22:16
@madphysicist
Copy link
Contributor Author

No more failing tests.

@madphysicist madphysicist force-pushed the test-fixture-fixup branch 2 times, most recently from f4b6f48 to 37592a7 Compare January 31, 2017 14:50
@dstansby dstansby changed the title MAINT: Updated tick and category test formatting [MRG+1] MAINT: Updated tick and category test formatting Jan 31, 2017
@madphysicist
Copy link
Contributor Author

@dstansby. I've been seeing [MRG+1] a lot recently on PR titles. What does it mean?

@story645
Copy link
Member

It means your PR has been approved by one reviewer (matplotlib asks for 2 approvals before commiting). http://matplotlib.org/devel/coding_guide.html#pr-review-guidelines

@madphysicist
Copy link
Contributor Author

Can you be the other one?

story645
story645 reviewed Jan 31, 2017
View reviewed changes
lib/matplotlib/tests/test_ticker.py Outdated
cases, especially the case when no SI prefix is present, for values in
[1, 1000).

Should not raise exceptions.
Copy link
Member

Choose a reason for hiding this comment

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

don't need this comment.

lib/matplotlib/tests/test_ticker.py Outdated

Should not raise exceptions.
"""
unitless = mticker.EngFormatter()
Copy link
Member

Choose a reason for hiding this comment

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

these should probably be two separate tests (unitless and with unit) and possibly parameterized

@story645
Copy link
Member

Sorry thought I approved. Have two comments, but if you wanna punt those to a different improve engineering test PR, let me know and I'll merge.

@madphysicist madphysicist force-pushed the test-fixture-fixup branch 2 times, most recently from 9d8d3ab to e3f721f Compare January 31, 2017 19:03
@madphysicist
Copy link
Contributor Author

@story645 I made the improvements here. I kept is as a single parametrized test, removed the extra comment and also took care of some extra whitespace in ticker.py.

@madphysicist madphysicist force-pushed the test-fixture-fixup branch 2 times, most recently from 85ee55b to 5211dc9 Compare January 31, 2017 19:49
@madphysicist
Copy link
Contributor Author

Not an actual failure, Mac test just didn't run.

@QuLogic
Copy link
Member

QuLogic commented Feb 1, 2017

With #7973 merged, this has conflicts. When you rebase, please be sure to leave out any empty cleanup decorators.

@story645
Copy link
Member

story645 commented Feb 1, 2017

I think on the rebase or something, you accidentally introduced a conflict. Please resolve.

@madphysicist
Copy link
Contributor Author

@QuLogic Should I replace @cleanup(style='classic') with @pytest.mark.style('classic'), and make all similar changes that make sense?

@QuLogic
Copy link
Member

QuLogic commented Feb 1, 2017

Classic is the default now, it's not necessary if that's really what's being used (and I realize I forgot to remove a couple.)

@madphysicist
Copy link
Contributor Author

In that case I will remove those too.

@madphysicist
Copy link
Contributor Author

Just to make triple sure, I should make the replacement for 'default' though?

@QuLogic
Copy link
Member

QuLogic commented Feb 1, 2017

That's correct.

@madphysicist
Copy link
Contributor Author

Done. I ended up parametrizing a couple of the tests and adding one pytest.mark.style('default') that I don't think was there before to make things pass. Should be good now.

@madphysicist
Copy link
Contributor Author

Again looks like Mac test didn't run. No actual failures.

@madphysicist madphysicist reopened this Feb 2, 2017
@madphysicist
Copy link
Contributor Author

I've closed and reopened the PR to restart Travis and hopefully get the Mac test to run, but if you approve, this PR is ready to go.

QuLogic
QuLogic requested changes Feb 2, 2017
View reviewed changes
lib/matplotlib/tests/test_ticker.py Outdated
9.441, 12.588])
assert_almost_equal(loc.tick_values(-7, 10), test_value)

def test_MultipleLocator_set_params(self):
Copy link
Member

Choose a reason for hiding this comment

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

Seems like you forgot to remove the class in this one.

lib/matplotlib/tests/test_ticker.py Outdated
fmt = ax.xaxis.get_major_formatter()
fmt.set_locs(ax.xaxis.get_majorticklocs())
show_major_labels = [fmt(x) != '' for x in
ax.xaxis.get_majorticklocs()]
Copy link
Member

Choose a reason for hiding this comment

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

Not aligned properly; better to wrap at for.

lib/matplotlib/tests/test_ticker.py Outdated
@pytest.mark.parametrize(
'labelOnlyBase, exponent, locs, positions, expected', param_data)
@pytest.mark.parametrize('base', base_data)
def test_LogFormatterExponent(self, labelOnlyBase, base, exponent,
Copy link
Member

Choose a reason for hiding this comment

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

Class name is still here.

lib/matplotlib/tests/test_ticker.py Outdated


class TestLogFormatterSciNotation(object):
testdata = [
Copy link
Member

Choose a reason for hiding this comment

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

Why testdata and not test_data like the others?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure if it would cause any problems if I started it with test_.

Copy link
Member

Choose a reason for hiding this comment

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

We'll see how the tests go, then, but I'd guess this shouldn't be a problem since it isn't callable.

@madphysicist
Copy link
Contributor Author

@QuLogic Made the following changes:

  • Removed 2 class names in method names.
  • Renamed testdata to test_data.
  • Wrapped list comprehension with for on new line.

@madphysicist
Copy link
Contributor Author

Ready to go?

@QuLogic QuLogic added this to the 2.1 (next point release) milestone Feb 2, 2017
@QuLogic QuLogic changed the title [MRG+1] MAINT: Updated tick and category test formatting MAINT: Updated tick and category test formatting Feb 2, 2017
@QuLogic QuLogic merged commit 7ffcca9 into matplotlib:master Feb 2, 2017
@madphysicist madphysicist deleted the test-fixture-fixup branch February 2, 2017 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@story645 story645 story645 approved these changes

@QuLogic QuLogic QuLogic approved these changes

@dstansby dstansby dstansby approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v2.1
Development

Successfully merging this pull request may close these issues.
4 participants
@madphysicist @story645 @QuLogic @dstansby