FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Support generative tests in @cleanup. by anntzer · Pull Request #5809 · matplotlib/matplotlib · GitHub

Repository navigation

Support generative tests in @cleanup. - #5809

Merged
mdboom merged 4 commits into
matplotlib:masterfrom
anntzer:cleanup-generative-tests
Jan 25, 2016
Merged

mdboom merged 4 commits into
matplotlib:masterfrom
anntzer:cleanup-generative-tests

Conversation

anntzer commented Jan 7, 2016

Copy link
Copy Markdown
Contributor

All's in the title.

anntzer commented Jan 7, 2016

Copy link
Copy Markdown
Contributor Author

Rebased #5785 onto this, just to show that this implementation works.

tacaswell added this to the next major release (2.0) milestone Jan 7, 2016

Copy link
Copy Markdown
Member

👍 that is way simpler than I expected it to be!

anntzer commented Jan 7, 2016

Copy link
Copy Markdown
Contributor Author

Hum... I am pretty sure that the cleanup decorator (in its old version) made nose silently skip test_axes.test_remove_shared_axes (the single failing test), which has then been (effectively) broken in some later commit. Can you investigate?

Copy link
Copy Markdown
Member

Huh, good catch. I will try to take a look at that tomorrow.

mdboom commented Jan 7, 2016

Copy link
Copy Markdown
Member

Thanks. We were just talking about this in #5718.

Copy link
Copy Markdown
Member

I do not have a clear memory of writing that test (but git blame tells me I did!). Sometimes I work in a tdd style where I just run the tests to check things so that test may have never worked.

For expediency of getting this merged, maybe throw a KnownFail on this create an issue telling me to fix the test.

anntzer commented Jan 7, 2016

Copy link
Copy Markdown
Contributor Author

Done. By the way, why was the previous AppVeyor build successful even though the test suite failed? (I am a bit confused.)

anntzer commented Jan 7, 2016

Copy link
Copy Markdown
Contributor Author

I am even more confused by the new test failure. Anyone wants to take over this PR? (TBH I have little interest in investigating this.)

Copy link
Copy Markdown
Member

Did you run these locally? That error is what happens if the test module is not importable.

Copy link
Copy Markdown
Member

Turns out the fix is trivial, there is just an extra assert before the assert_array_equal. The function was not raising, so returning None which fails the assert.

Copy link
Copy Markdown
Member

I am going to do some local git merging to:

  • drop the second commit of this PR
  • add a commit to fix the test
  • backport the whole thing to v1.5.x

Copy link
Copy Markdown
Member

Actually, I am having issues getting this to pass locally and am getting a large number of file not closed warnings, see anntzer#1

anntzer commented Jan 8, 2016

Copy link
Copy Markdown
Contributor Author

Actually my original patch was wrong... can you see why? It's fixed now.

mdboom added a commit that referenced this pull request Jan 25, 2016
mdboom merged commit b3e576a into matplotlib:master Jan 25, 2016
mdboom modified the milestones: Critical bug fix release (1.5.2), next major release (2.0) Jan 25, 2016
tacaswell pushed a commit that referenced this pull request Mar 26, 2016

Copy link
Copy Markdown
Member

backported to v2.x as 80be853

tacaswell added this to the 2.0 (style change major release) milestone Mar 26, 2016
tacaswell removed this from the 1.5.2 (Critical bug fix release) milestone Mar 26, 2016
bearstrong pushed a commit to bearstrong/matplotlib that referenced this pull request Apr 1, 2016
anntzer deleted the cleanup-generative-tests branch June 10, 2016 06:23
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants


Back | FazBrowse Home | New Git URL