test: skip the PNG assertions when rsvg-convert is absent - #81
Merged
Conversation
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
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.
Why
ci.ymlstates, above thelibrsvg2-bininstall: "Without it every PNG assertion skips itself, sothe version check is what keeps the job from passing empty." That was not true. Five tests failed
rather than skipping on a machine without
rsvg-convert:test_lockups.py::test_render_writes_three_lockup_assetstest_pngopt.py::test_social_card_png_is_quantized_and_smalltest_pngopt.py::test_transparent_mark_png_keeps_alpha_and_is_smalltest_pngopt.py::test_card_palette_is_actually_reducedtest_projects.py::test_render_projects_writes_lockupexport_pngreturnsFalseand writes nothing when the binary is absent, so these assert on PNGsthat were never produced. The effect is a suite that cannot go green on a contributor's machine
unless they install librsvg first, which nothing tells them to do outside
brand/README.md.Design
No new mechanism: this uses the two patterns the suite already has, picking per test by whether the
test is wholly about PNG output or only partly.
test_pngopt.pyis wholly PNG-dependent — every test opens a generated PNG — so it takes amodule-level
pytestmark = pytest.mark.skipif(...), the same guard already ontest_projects.py::test_no_cream_on_transparent.test_lockups.py::test_render_writes_three_lockup_assetsandtest_projects.py::test_render_projects_writes_lockupare mixed: each asserts two SVGs and thena PNG. They take an inline
if shutil.which("rsvg-convert"):around the PNG half only, the sameshape used six times in
test_assets.py. Their SVG assertions keep running everywhere.Skipping the mixed tests wholesale would have been the smaller diff and the wrong one: it would have
dropped live SVG coverage to fix a PNG problem.
Non-goals
brand/build/raster.pyis unchanged.export_pngreturningFalsewithout the binary is correctand is what makes the guard meaningful.
Nothing in CI changes. The
rsvg-convert --versionstep already fails the job when the binary ismissing, so these guards cannot hollow the job out — that pre-existing check is exactly what makes
skipping safe here, and it is why the comment describing this behaviour was worth making true.
Verification
Locally, without
rsvg-convertinstalled:mainPassing goes up, not down: the three
test_pngopt.pytests skip (26 → 29), and the two mixedtests now pass on their SVG assertions instead of failing (123 → 125). No test is disabled.
The with-binary path is what CI exercises, and it is unchanged by construction — every guard is a
shutil.whichthat is truthy there. CI on this PR is the check for that, since the machine this waswritten on has no librsvg.