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.

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

Support generative tests in @cleanup. #5809

Merged
merged 4 commits into from Jan 25, 2016

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Jan 7, 2016

All's in the title.

@anntzer
Copy link
Contributor Author

anntzer commented Jan 7, 2016

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

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

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

@anntzer
Copy link
Contributor Author

anntzer commented Jan 7, 2016

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?

@tacaswell
Copy link
Member

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

@mdboom
Copy link
Member

mdboom commented Jan 7, 2016

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

@tacaswell
Copy link
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
Copy link
Contributor Author

anntzer commented Jan 7, 2016

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

@anntzer
Copy link
Contributor Author

anntzer commented Jan 7, 2016

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.)

@tacaswell
Copy link
Member

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

@tacaswell
Copy link
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.

@tacaswell
Copy link
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

@tacaswell
Copy link
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
Copy link
Contributor Author

anntzer commented Jan 8, 2016

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 mdboom merged commit b3e576a into matplotlib:master Jan 25, 2016
@mdboom 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
@tacaswell
Copy link
Member

backported to v2.x as 80be853

@tacaswell tacaswell added this to the 2.0 (style change major release) milestone Mar 26, 2016
@tacaswell 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 anntzer deleted the cleanup-generative-tests branch June 10, 2016 06:23
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.

None yet

4 participants