NotificationsYou must be signed in to change notification settings
Fork7.9k
Star21.4k

Fix undefined name. Add animation tests.#10801

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

jklymak merged 2 commits intomatplotlib:masterfromanntzer:fix-undefined-name

Mar 21, 2018

Merged

Fix undefined name. Add animation tests.#10801

jklymak merged 2 commits intomatplotlib:masterfromanntzer:fix-undefined-name

Mar 21, 2018

Conversation

Copy link

Contributor

PR Summary

xref#10793 (comment)
Sorry :)

PR Checklist

Has Pytest style unit tests
Code is PEP 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

anntzer added the Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label

Mar 15, 2018

anntzer added this to thev3.0 milestone

Mar 15, 2018

afvincent approved these changes

Mar 15, 2018

View reviewed changes

Copy link

Contributor

afvincent left a comment

Choose a reason for hiding this comment

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

LGTM.

If this undefined variable never was an issue, does that mean that this code path is never exercised during the tests (genuine question)?

Copy link

Member

tacaswell commentedMar 15, 2018

Yeah, we do not force it to try to save a too-big animation to js.

tacaswell approved these changes

Mar 15, 2018

View reviewed changes

Copy link

Contributor

dopplershift commentedMar 15, 2018

Oops. 🐑

Copy link

Member

jklymak commentedMar 15, 2018

... I told@anntzer that I trusted him... and the tests.

Copy link

Contributor

dopplershift commentedMar 15, 2018

Yeah, I see now that I didn't screw it up originally. 😁

Copy link

ContributorAuthor

anntzer commentedMar 15, 2018•
edited
Loading

Well, I'm writing tests for the whole thing right now and it looks like (tbc?) I'm finding an unrelated bug in html5 output... so perhaps that was a good thing that I'm breaking stuff here and there :p
edit: oh, it's just an instance of#9205.

Copy link

ContributorAuthor

anntzer commentedMar 15, 2018•
edited
Loading

Added some tests, including an xfailing one :-)
btw I think we should replace most instances of xfail in the test suite by skipifs, so that xfail really means "this is a bug that we need to fix", not "the preconditions for running the test (e.g. OS, 3rd-party module) are not satisfied".