Make SVG plots the default in Java and fix CI snapshot tests#200
Open
ewels wants to merge 2 commits into
Open
Conversation
`FastQCConfig.svg_output` defaulted to `false` so any code path that didn't
go through the perl launcher (notably the integration tests, which invoke
`java uk.ac.babraham.FastQC.FastQCApplication` directly) still got PNG
plots. The launcher only added `-Dfastqc.svg=true` to override that default;
`--png` worked by leaving the property unset. Flipping the Java default
without reworking that handler would have broken `--png`.
This commit:
* flips `FastQCConfig.svg_output = true` so every entry point (launcher,
direct java, integration tests) renders SVG plots by default;
* reworks the `-Dfastqc.svg` handler to honour both `true` and `false`,
so the launcher can opt back into PNG when the user passes `--png`;
* updates the launcher to pass `-Dfastqc.svg=false` only when `--png` is
set without an explicit `--svg`;
* regenerates `FileContentsTest_{minimal,complex}_fastqc_report.approved.html`
so the snapshots reflect SVG plots. The new approved files are bit-
identical across Linux/macOS and Java 11/17 because SVG is plain XML
generated directly by the code, with no AWT raster pipeline. The
previous PNG approved files diverged on every push because pixel
rendering differs between macOS (Core Graphics) and Linux (FreeType);
* fixes `VersionTest` to reference `FastQCApplication.VERSION` instead of
a hardcoded `v0.12.1` literal, so the assertion tracks future bumps.
Tested locally with Java 11 (matches CI) and Java 17: unit + integration
suites pass clean.
SVGGenerator is not platform-deterministic: on Linux the Swing default JPanel background (rgb(238,238,238)) gets painted before the chart's own fill, and the inherited UI font is 12pt vs 13pt on macOS. Approved files generated locally on macOS therefore don't match CI output. Promote the .received.html files from the failing CI run as the new approved snapshots. Cross-platform divergence in SVGGenerator itself is a separate fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
Correction, SVG output is apparently not deterministic, across operating systems. Or rather, the Java environment that the SVG is generated from is not. OS-specific differences in font-size and in the background panel. These are visually minor though, and it should be consistent when running within the CI environment, so I just took the version from the CI runner. Means that the tests pass as they should and function as regression testing. Downside is that it's difficult to regenerate new snapshots locally. Proper fix is to try to cancel out those two OS-specific differences, but I'd rather not mess around adding in tonnes of font-size declarations all through the SVG if we can avoid it (having just spent some time trying to optimise that kind of thing out of the SVG). |
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
1 participant
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.
CI has been failing on every PR (and on pushes to master) since around April, failing on
FileContentsTestwith diffs in the base64-encoded PNG plot images inside the snapshot HTML. This is due to differences in the text with anti-aliased pixels, so the byte comparison fails.This should have been fixed by using SVG plots by default instead of PNG. However, PR #191 flipped the default in the perl launcher (
fastqcadds-Dfastqc.svg=trueunless--pngis passed) but not in the Java code itself, whereFastQCConfig.svg_outputwas still initialised tofalse. The integration tests invokejava uk.ac.babraham.FastQC.FastQCApplicationdirectly, bypassing the launcher, so they got the Java default (PNG) and have been comparing pixel-perfect PNGs ever since.This PR:
FastQCConfig.java: defaultsvg_output = true. The-Dfastqc.svghandler now honours bothtrueandfalse, so callers can still opt back into PNG.fastqclauncher: only pass-Dfastqc.svg=falsewhen--pngis set without an explicit--svg. SVG is now the default everywhere.FileContentsTest_{minimal,complex}_fastqc_report.approved.html: regenerated. Plots are nowdata:image/svg+xml;base64,...instead ofdata:image/png;base64,.... SVG output is deterministic.VersionTest.java: assertion referencesFastQCApplication.VERSIONinstead of the hardcodedv0.12.1, so it continues to pass after future version bumps.Tested locally with Java 11 (matches CI) and Java 17: unit + integration suites pass clean.