Fix multi.dot Graphviz regression test #211
Merged
Conversation
This file contains 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
Commit d6602ad of 2018-12-01 fixed the regression test broken by commit 2d55978 of 2016-07-01. This revealed that `test/graphs/multi.dot` was failing. `multi.dot` was introduced in commit 2b3f088 of 2010-11-07 together with many of the other tests still here today. It has not been touched since. It is a DOT-file containing two digraphs. The regression test compares the JPEG images rendered from the DOT-file by pydot with those rendered by Graphviz's dot directly. Commit 66734d2 of 2016-07-01 is the actual cause of the failure. It changed one of the render methods of the regression test, `_render_with_pydot`, from calculating a single hash for all the JPEG images to calculating separate hashes for each JPEG image and then concatenating those hashes in one long string. The other render method, `_render_with_graphviz`, still calculates a single hash over all data. For DOT-files that generate only one image the end result is the same, but because `multi.dot` has two graphs, it produces two images and this leads to comparing a string of two hashes with one single hash. I do not think the change in generating the hash was intentional, for the following reasons: - Commit 66734d2 states that its purpose was to adapt the test to an API change in pydot. It does not mention a deliberate choice to change the testing method. - There was no effort to change `_render_with_graphviz` to also produce multiple hashes. - Except for easier debugging in case of a failing test with multiple images (AFAICT, only `multi.dot`), I do not see much added benefit in checking a concatenation of the hashes of all images vs. checking one hash of all images together: In both cases the test will fail if one or more images is rendered differently. - Given that there were many commits authored that same hour, including commit 2d55978 which broke the regression tests, I suspect the author did not run the tests for each individual commit, but only at the end of that batch, and was therefore also not alerted of this change by the test suite. Assuming that the change was not intended, this commit will now revert `_render_with_pydot` to the old behavior of calculating a single hash from all JPEG image data. Tested with Debian 9.9, Graphviz 2.38.0-17, Python 2.7.13-2 and 3.5.3-1. Fixes pydot#204.
|
Not sure why tests are failing in AppVeyor. All tests pass successful with this PR in my setup. Some more details: |
|
As I describe in issue #213, I think the test failures in AppVeyor can be ignored. They seem to be caused by some nondeterministic behavior that not only affects the pydot tests, but also Graphviz This PR (#211) is still necessary to fix the |
Closed
|
This indeed fixes the unittest failure, please merge |
This was referenced May 11, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Commit d6602ad of 2018-12-01 fixed the regression test broken by commit 2d55978 of 2016-07-01. This revealed that
test/graphs/multi.dotwas failing.multi.dotwas introduced in commit 2b3f088 of 2010-11-07 together with many of the other tests still here today. It has not been touched since. It is a DOT-file containing two digraphs. The regression test compares the JPEG images rendered from the DOT-file by pydot with those rendered by Graphviz's dot directly.Commit 66734d2 of 2016-07-01 is the actual cause of the failure. It changed one of the render methods of the regression test,
_render_with_pydot, from calculating a single hash for all the JPEG images to calculating separate hashes for each JPEG image and then concatenating those hashes in one long string. The other render method,_render_with_graphviz, still calculates a single hash over all data. For DOT-files that generate only one image the end result is the same, but becausemulti.dothas two graphs, it produces two images and this leads to comparing a string of two hashes with one single hash.I do not think the change in generating the hash was intentional, for the following reasons:
_render_with_graphvizto also produce multiple hashes.multi.dot), I do not see much added benefit in checking a concatenation of the hashes of all images vs. checking one hash of all images together: In both cases the test will fail if one or more images is rendered differently.Assuming that the change was not intended, this commit will now revert
_render_with_pydotto the old behavior of calculating a single hash from all JPEG image data.Tested with Debian 9.9, Graphviz 2.38.0-17, Python 2.7.13-2 and 3.5.3-1.
Fixes #204.