Fix #5495. Combine two tests to prevent race cond #5656

Merged
merged 1 commit into from Dec 11, 2015

Conversation

Projects
None yet
4 participants
Owner

mdboom commented Dec 10, 2015

There are two tests in test_cycles.py that use the same baseline image. If running the tests in multiprocess mode, it's theoretically possible that both tests would write to the output file at the same time, corrupting the file and hence the backtrace when reading it back in. I think combining these into a single test with the same image file will accomplish what is intended (sharing the same baseline data) but never run in parallel.

@mdboom mdboom fix #5495. Combine two tests to prevent race cond
1c14132

mdboom added the needs_review label Dec 10, 2015

Owner

mdboom commented Dec 10, 2015

The nice thing is is that if my theory is correct, this is a test-only bug, and not really a bug in matplotlib itself.

Longer term, we could add some sort of hook to the testing infrastructure so that filenames can't be reused or so that writes are atomic.

Member

WeatherGod commented Dec 10, 2015

I think your hypothesis is correct for the theoretical race condition, but I don't see how this is any better. Wouldn't the results get saved out to the same named file? So if the first figure had a regression in it, the second figure could clobber that.

Owner

mdboom commented Dec 10, 2015

Yes, the results get saved out to the same file, but they are compared one-at-a-time. So both are tested against the same baseline image in turn. It does mean that the first one to fail is the one that's left on disk for inspection later, and there may be other failures "beyond" that, but that's no worse than the usual situation with asserts in unit tests. We have a number of tests that do this already without problem.

Member

WeatherGod commented Dec 10, 2015

ah, ok. That makes sense. Just waiting for Travis to greenlight this.

@jenshnielsen jenshnielsen added a commit that referenced this pull request Dec 11, 2015

@jenshnielsen jenshnielsen Merge pull request #5656 from mdboom/imread-bug
Fix #5495.  Combine two tests to prevent race cond
bdb8450

@jenshnielsen jenshnielsen merged commit bdb8450 into matplotlib:master Dec 11, 2015

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 68.25%
Details

tacaswell removed the needs_review label Dec 11, 2015

@jenshnielsen jenshnielsen added a commit that referenced this pull request Dec 11, 2015

@jenshnielsen @jenshnielsen jenshnielsen + jenshnielsen Merge pull request #5656 from mdboom/imread-bug
Fix #5495.  Combine two tests to prevent race cond
dfadd4d
Owner

jenshnielsen commented Dec 11, 2015

back ported as dfadd4d

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment