Snapshot tests - #2098
Conversation
Thanks for your contribution! 🤗 (1 / 4)The build and test process is starting. This may take a while. Packing complete! 📦 (2 / 4)You can download the NuGet packages for this build here. Running tests 🧪 (3 / 4)Tests are running now, you can monitor their progress below or at the actions tab. Core ✅ | Snapshot ❌ | Windows ✅ | Linux ✅ | Mac ✅ | Browser ✅ | Android ✅ | iOS ✅ Test Results Summary (Failure) ❌ (4 / 4) |
There was a problem hiding this comment.
Pull request overview
This PR introduces snapshot testing capabilities to the LiveCharts library by adding approximately 60 new visual regression tests. The snapshot testing methodology compares generated chart images against committed baseline images using a threshold-based comparison approach (2 per-channel tolerance, 0.1% different pixels allowed). When chart rendering changes, developers must explicitly commit new baseline images to update the tests.
Changes:
- New snapshot testing infrastructure with image comparison logic
- 60+ new test methods covering tooltips, legends, various chart types, axes, special cases, and effects
- Thread safety fix for font shaper caching to support parallel test execution
- CI/CD workflow integration for automated snapshot test execution
Reviewed changes
Copilot reviewed 29 out of 96 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/SnapshotTests/SnapshotTests.csproj | Test project configuration targeting net10.0 with MSTest runner |
| tests/SnapshotTests/Extensions.cs | Custom extension methods for snapshot comparison and chart pointer simulation |
| tests/SnapshotTests/*.cs (18 test files) | Test implementations covering different chart features and series types |
| tests/SnapshotTests/Snapshots/*.png | Baseline snapshot images for comparison |
| src/skiasharp/LiveChartsCore.SkiaSharp/SKCharts/InMemorySkiaSharpChart.cs | Background color handling update to support custom backgrounds in tests |
| src/skiasharp/LiveChartsCore.SkiaSharp/Drawing/DrawingTextExtensions.cs | Thread safety fix for font shaper dictionary access |
| src/LiveChartsCore/CartesianChartEngine.cs | Crosshair invalidation logic for proper tooltip rendering |
| src/LiveChartsCore/AssemblyInfo.cs | InternalsVisibleTo attribute for test access |
| .github/workflows/livecharts.yml | CI workflow integration for snapshot tests |
| .github/actions/run-tests/action.yml | Test runner action updates to support snapshot test type |
| global.json | Test runner configuration |
| LiveCharts.slnx | Solution file update to include snapshot test project |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Thanks for your contribution! 🤗 (1 / 4)The build and test process is starting. This may take a while. Packing complete! 📦 (2 / 4)You can download the NuGet packages for this build here. Running tests 🧪 (3 / 4)Tests are running now, you can monitor their progress below or at the actions tab. |
Thanks for your contribution! 🤗 (1 / 4)The build and test process is starting. This may take a while. Packing complete! 📦 (2 / 4)You can download the NuGet packages for this build here. Running tests 🧪 (3 / 4)Tests are running now, you can monitor their progress below or at the actions tab. |
This reverts commit b708edf.
Thanks for your contribution! 🤗 (1 / 4)The build and test process is starting. This may take a while. Packing complete! 📦 (2 / 4)You can download the NuGet packages for this build here. Running tests 🧪 (3 / 4)Tests are running now, you can monitor their progress below or at the actions tab. Core ✅ | Snapshot ✅ | Windows ✅ | Linux ✅ | Mac ✅ | Browser ✅ | Android ✅ | iOS ✅ Test Results Summary (Passed) ✅ (4 / 4) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 97 changed files in this pull request and generated 9 comments.
Comments suppressed due to low confidence (1)
src/skiasharp/LiveChartsCore.SkiaSharp/Drawing/DrawingTextExtensions.cs:301
s_knownShapersnow uses a global lock only for dictionary access, but the returnedSKShaperinstance is shared andshaper.Shape(...)is called outside the lock. IfSKShaperis not thread-safe (common for Skia objects), parallel tests can still race. Consider either locking around theShapecall (or per-typeface lock), or avoid sharingSKShaperinstances across threads (e.g., ThreadLocal or create-per-call with caching of typefaces only).
SKShaper? shaper = null;
lock (s_lock)
{
if (!s_knownShapers.TryGetValue(typeface.FamilyName, out shaper))
{
shaper = new SKShaper(typeface);
s_knownShapers[typeface.FamilyName] = shaper;
}
}
var result = shaper.Shape(text, paint);
var glyphs = Array.ConvertAll(result.Codepoints, cp => (ushort)cp);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -63,14 +63,14 @@ runs: | |||
|
|
|||
| # run tests using the factos selection inside the android emulator | |||
| - name: Enable KVM | |||
| if : ${{ inputs.use-factos == 'true' && inputs.use-android-emulator == 'true' }} | |||
| if : ${{ inputs.test-type == 'factos' && inputs.use-android-emulator == 'true' }} | |||
| shell: pwsh | |||
| run: | | |||
| echo 'KERNEL=="kvm", GROUP="kvm", MODE="0666", OPTIONS+="static_node=kvm"' | sudo tee /etc/udev/rules.d/99-kvm4all.rules | |||
| sudo udevadm control --reload-rules | |||
| sudo udevadm trigger --name-match=kvm | |||
| - name: Create Test Script | |||
| if : ${{ inputs.use-factos == 'true' && inputs.use-android-emulator == 'true' }} | |||
| if : ${{ inputs.test-type == 'factos' && inputs.use-android-emulator == 'true' }} | |||
| shell: bash | |||
| run: | | |||
| cat << 'EOF' > run_tests.sh | |||
| @@ -86,29 +86,39 @@ runs: | |||
| EOF | |||
| chmod +x run_tests.sh | |||
| - name: Start Android emulator and run tests | |||
| if : ${{ inputs.use-factos == 'true' && inputs.use-android-emulator == 'true' }} | |||
| if : ${{ inputs.test-type == 'factos' && inputs.use-android-emulator == 'true' }} | |||
| uses: reactivecircus/android-emulator-runner@v2.35.0 | |||
| with: | |||
| api-level: 35 | |||
| target: google_apis | |||
| arch: x86_64 | |||
| script: ./run_tests.sh | |||
|
|
|||
| # run core tests when factos is not used | |||
| # the Debug config is required even we are testing the nuget packages in Release. | |||
| # this is because some optimizations in Release make the tests fail, but the code | |||
| # that fails is not related to the nuget packages but to the test code itself | |||
| # there is a function that mocks the ui drawing that fails in Release optimizations. | |||
| # ToDo: fix that, or now that we have ui tests, maybe just run the tests in the ui?. | |||
| - name: Run tests | |||
| - name: Run core tests | |||
| uses: nick-fields/retry@v3.0.2 | |||
| if : ${{ inputs.use-factos != 'true' }} | |||
| if : ${{ inputs.test-type == 'core' }} | |||
| with: | |||
| timeout_minutes: 8 | |||
| max_attempts: 3 | |||
| shell: pwsh | |||
| command: dotnet run --project tests/CoreTests/CoreTests.csproj -c Debug -f ${{ inputs.target-framework }} --report-trx -p:UseNuGetPackages=true -p:LiveChartsVersionSuffix=-${{ github.sha }} | |||
|
|
|||
| - name: Run snapshot tests | |||
| uses: nick-fields/retry@v3.0.2 | |||
| if : ${{ inputs.test-type == 'snapshot' }} | |||
| with: | |||
There was a problem hiding this comment.
The step conditions use if : (with a space) instead of the supported if: key. GitHub Actions will treat if as an unknown key, so these conditions may be ignored and steps could run unexpectedly. Rename if : to if: for all affected steps in this file.
| - name: Report tests completion | ||
| if: ${{ always() && !contains(join(needs.*.result, ' '), 'failure') && !contains(join(needs.*.result, ' '), 'cancelled') }} | ||
| uses: ./.github/actions/comment-progress | ||
| with: | ||
| comment_id: ${{ needs.report-progress-starting.outputs.comment_id }} | ||
| content: | | ||
| core #${{ needs.test-core.result }}, windows #${{ needs.test-windows.result }}, linux #${{ needs.test-linux.result }}, mac #${{ needs.test-mac.result }}, browser #${{ needs.test-browser.result }}, android #${{ needs.test-android.result }}, ios #${{ needs.test-ios.result }} | ||
| Core #${{ needs.test-core.result }} | Snapshot #${{ needs.test-snapshot.result }} | Windows #${{ needs.test-windows.result }} | Linux #${{ needs.test-linux.result }} | Mac #${{ needs.test-mac.result }} | Browser #${{ needs.test-browser.result }} | Android #${{ needs.test-android.result }} | iOS #${{ needs.test-ios.result }} | ||
|
|
||
| ### Test Results Summary (Passed) ✅ (4 / 4) | ||
|
|
||
| - name: Report tests failure | ||
| if: ${{ always() && (contains(join(needs.*.result, ' '), 'failure') || contains(join(needs.*.result, ' '), 'cancelled')) }} | ||
| uses: ./.github/actions/comment-progress | ||
| with: | ||
| comment_id: ${{ needs.report-progress-starting.outputs.comment_id }} | ||
| content: | | ||
|
|
||
| Core #${{ needs.test-core.result }} | Windows #${{ needs.test-windows.result }} | Linux #${{ needs.test-linux.result }} | Mac #${{ needs.test-mac.result }} | Browser #${{ needs.test-browser.result }} | Android #${{ needs.test-android.result }} | iOS #${{ needs.test-ios.result }} | ||
| Core #${{ needs.test-core.result }} | Snapshot #${{ needs.test-snapshot.result }} | Windows #${{ needs.test-windows.result }} | Linux #${{ needs.test-linux.result }} | Mac #${{ needs.test-mac.result }} | Browser #${{ needs.test-browser.result }} | Android #${{ needs.test-android.result }} | iOS #${{ needs.test-ios.result }} | ||
|
|
||
| ### Test Results Summary (Failure) ❌ (4 / 4) |
There was a problem hiding this comment.
The progress comment still says "(4 / 4)" even though the workflow now has 8 test jobs (Core, Snapshot, Windows, Linux, Mac, Browser, Android, iOS). This will make the status message misleading; update the counts (or compute them dynamically) to match the actual number of required test jobs.
| Path.Combine("SnapshotsDiff", $"{name}[EXPECTED].png")); | ||
|
|
||
| File.Copy( | ||
| Path.Combine("SnapshotsNew", $"{name}.png"), | ||
| Path.Combine("SnapshotsDiff", $"{name}[RESULT].png")); |
There was a problem hiding this comment.
File.Copy is called without overwrite:true. Because the GitHub Action wraps the run in a retry loop, a second attempt in the same workspace can fail with an IOException if the diff files already exist, masking the real snapshot mismatch. Use overwrite (or delete existing files) when writing into SnapshotsDiff.
| Path.Combine("SnapshotsDiff", $"{name}[EXPECTED].png")); | |
| File.Copy( | |
| Path.Combine("SnapshotsNew", $"{name}.png"), | |
| Path.Combine("SnapshotsDiff", $"{name}[RESULT].png")); | |
| Path.Combine("SnapshotsDiff", $"{name}[EXPECTED].png"), | |
| overwrite: true); | |
| File.Copy( | |
| Path.Combine("SnapshotsNew", $"{name}.png"), | |
| Path.Combine("SnapshotsDiff", $"{name}[RESULT].png"), | |
| overwrite: true); |
| var ePtr = (byte*)expectedPixmap.GetPixels(); | ||
| var aPtr = (byte*)actualPixmap.GetPixels(); | ||
|
|
||
| var byteCount = totalPixels * 4; // RGBA | ||
|
|
||
| for (var i = 0; i < byteCount; i += 4) | ||
| { | ||
| var eR = ePtr[i + 0]; | ||
| var eG = ePtr[i + 1]; | ||
| var eB = ePtr[i + 2]; | ||
| var eA = ePtr[i + 3]; | ||
|
|
||
| var aR = aPtr[i + 0]; | ||
| var aG = aPtr[i + 1]; | ||
| var aB = aPtr[i + 2]; | ||
| var aA = aPtr[i + 3]; | ||
|
|
||
| var isDifferent = | ||
| Math.Abs(eR - aR) > perChannelTolerance || | ||
| Math.Abs(eG - aG) > perChannelTolerance || | ||
| Math.Abs(eB - aB) > perChannelTolerance || | ||
| Math.Abs(eA - aA) > perChannelTolerance; | ||
|
|
||
| if (isDifferent) | ||
| { | ||
| differentPixels++; | ||
|
|
||
| if (diffBitmap != null) | ||
| { | ||
| var pixelIndex = i / 4; | ||
| var x = pixelIndex % width; | ||
| var y = pixelIndex / width; | ||
|
|
||
| diffBitmap.SetPixel(x, y, new SKColor(255, 0, 0)); // red highlight | ||
| } | ||
|
|
||
| // Early exit if too many differences | ||
| if ((double)differentPixels / totalPixels > maxDifferentPixelsRatio) | ||
| break; | ||
| } | ||
| else if (diffBitmap != null) | ||
| { | ||
| var pixelIndex = i / 4; | ||
| var x = pixelIndex % width; | ||
| var y = pixelIndex / width; | ||
|
|
||
| var gray = (byte)((eR + eG + eB) / 3); | ||
| diffBitmap.SetPixel(x, y, new SKColor(gray, gray, gray)); | ||
| } |
There was a problem hiding this comment.
The pixel comparison assumes PeekPixels() returns 4 bytes per pixel in RGBA order (byteCount = totalPixels * 4 and ePtr[i+0..3]). SKPixmap color type/channel order isn’t guaranteed to be RGBA (often BGRA/Premul), so this can produce false diffs. Consider reading pixels via SKPixmap.ColorType/BytesPerPixel (and/or GetPixelColor) or normalizing both images into a known SKColorType before comparing.
| var ePtr = (byte*)expectedPixmap.GetPixels(); | |
| var aPtr = (byte*)actualPixmap.GetPixels(); | |
| var byteCount = totalPixels * 4; // RGBA | |
| for (var i = 0; i < byteCount; i += 4) | |
| { | |
| var eR = ePtr[i + 0]; | |
| var eG = ePtr[i + 1]; | |
| var eB = ePtr[i + 2]; | |
| var eA = ePtr[i + 3]; | |
| var aR = aPtr[i + 0]; | |
| var aG = aPtr[i + 1]; | |
| var aB = aPtr[i + 2]; | |
| var aA = aPtr[i + 3]; | |
| var isDifferent = | |
| Math.Abs(eR - aR) > perChannelTolerance || | |
| Math.Abs(eG - aG) > perChannelTolerance || | |
| Math.Abs(eB - aB) > perChannelTolerance || | |
| Math.Abs(eA - aA) > perChannelTolerance; | |
| if (isDifferent) | |
| { | |
| differentPixels++; | |
| if (diffBitmap != null) | |
| { | |
| var pixelIndex = i / 4; | |
| var x = pixelIndex % width; | |
| var y = pixelIndex / width; | |
| diffBitmap.SetPixel(x, y, new SKColor(255, 0, 0)); // red highlight | |
| } | |
| // Early exit if too many differences | |
| if ((double)differentPixels / totalPixels > maxDifferentPixelsRatio) | |
| break; | |
| } | |
| else if (diffBitmap != null) | |
| { | |
| var pixelIndex = i / 4; | |
| var x = pixelIndex % width; | |
| var y = pixelIndex / width; | |
| var gray = (byte)((eR + eG + eB) / 3); | |
| diffBitmap.SetPixel(x, y, new SKColor(gray, gray, gray)); | |
| } | |
| for (var y = 0; y < height; y++) | |
| { | |
| for (var x = 0; x < width; x++) | |
| { | |
| var expectedColor = expectedPixmap.GetPixelColor(x, y); | |
| var actualColor = actualPixmap.GetPixelColor(x, y); | |
| var eR = expectedColor.Red; | |
| var eG = expectedColor.Green; | |
| var eB = expectedColor.Blue; | |
| var eA = expectedColor.Alpha; | |
| var aR = actualColor.Red; | |
| var aG = actualColor.Green; | |
| var aB = actualColor.Blue; | |
| var aA = actualColor.Alpha; | |
| var isDifferent = | |
| Math.Abs(eR - aR) > perChannelTolerance || | |
| Math.Abs(eG - aG) > perChannelTolerance || | |
| Math.Abs(eB - aB) > perChannelTolerance || | |
| Math.Abs(eA - aA) > perChannelTolerance; | |
| if (isDifferent) | |
| { | |
| differentPixels++; | |
| if (diffBitmap != null) | |
| { | |
| diffBitmap.SetPixel(x, y, new SKColor(255, 0, 0)); // red highlight | |
| } | |
| // Early exit if too many differences | |
| if ((double)differentPixels / totalPixels > maxDifferentPixelsRatio) | |
| break; | |
| } | |
| else if (diffBitmap != null) | |
| { | |
| var gray = (byte)((eR + eG + eB) / 3); | |
| diffBitmap.SetPixel(x, y, new SKColor(gray, gray, gray)); | |
| } | |
| } | |
| // If we've already exceeded the max difference ratio, exit outer loop as well | |
| if ((double)differentPixels / totalPixels > maxDifferentPixelsRatio) | |
| break; |
| var bg = Background == SKColors.Empty | ||
| ? coreChart.GetTheme().VirtualBackroundColor.AsSKColor() | ||
| : Background; |
There was a problem hiding this comment.
Using SKColors.Empty as a sentinel for “use theme background” prevents consumers from requesting a transparent background, because SKColors.Empty is also the natural way to represent fully transparent. Consider switching Background to SKColor? (null = use theme), or adding an explicit UseThemeBackground flag so transparent backgrounds remain possible.
| var bg = Background == SKColors.Empty | |
| ? coreChart.GetTheme().VirtualBackroundColor.AsSKColor() | |
| : Background; | |
| var bg = Background | |
| ?? coreChart.GetTheme().VirtualBackroundColor.AsSKColor(); |
| if (axis.IsVisible) | ||
| { | ||
| AddVisual(axis.ChartElementSource); | ||
| axis.InvalidateCrosshair(this, _pointerPosition); |
There was a problem hiding this comment.
Measure() now calls axis.InvalidateCrosshair(this, _pointerPosition) for every visible axis. InvalidateCrosshair does not check chart._isPointerIn, so crosshairs may get re-created during measure even after the pointer has left (pointer position remains inside draw margin). Consider gating this call on _isPointerIn (and/or clearing crosshairs when _isPointerIn is false) to avoid crosshair visuals sticking around unexpectedly.
| axis.InvalidateCrosshair(this, _pointerPosition); | |
| if (_isPointerIn) | |
| { | |
| axis.InvalidateCrosshair(this, _pointerPosition); | |
| } |
| // define the danger state, a red fill. | ||
| _ = columnSeries.HasState("Danger", [ | ||
| (nameof(IDrawnElement.Fill), new SolidColorPaint(SKColors.Yellow)) | ||
| ]); | ||
|
|
||
| _ = columnSeries.HasState("LabelDanger", [ | ||
| (nameof(IDrawnElement.Paint), new SolidColorPaint(SKColors.Yellow)), | ||
| (nameof(BaseLabelGeometry.TextSize), 30f), |
There was a problem hiding this comment.
Comment says the "danger state" is a red fill, but the code sets SKColors.Yellow for both the fill and label paint. Update the comment or the color so the test intent matches the implementation.
| var chart = new SKCartesianChart | ||
| { | ||
| Series = [ | ||
| new ColumnSeries<ErrorValue> | ||
| { | ||
| Values = values1, | ||
| ShowError = true | ||
| }, | ||
| ], |
There was a problem hiding this comment.
The LineWithError test is using ColumnSeries<ErrorValue> (same as the column test). If the intention is to validate line-series error rendering, this should likely be a LineSeries<ErrorValue> (or the test should be renamed to match the behavior).
| [TestMethod] | ||
| public void LabelsFormat() | ||
| { | ||
| double[] customSeparators = [0, 10, 25, 50, 100]; | ||
|
|
||
| var chart = new SKCartesianChart | ||
| { | ||
| Series = [ | ||
| new LineSeries<double> { Values = [10, 55, 45, 68, 60, 70, 75, 120] } | ||
| ], | ||
| XAxes = [ | ||
| new Axis | ||
| { | ||
|
|
||
| } | ||
| ], | ||
| YAxes = [ | ||
| new Axis | ||
| { | ||
| CustomSeparators = customSeparators | ||
| } | ||
| ], | ||
| Width = 600, | ||
| Height = 600 | ||
| }; | ||
|
|
||
| chart.AssertSnapshotMatches($"{nameof(AxesTests)}_{nameof(LabelsFormat)}"); | ||
| } |
There was a problem hiding this comment.
LabelsFormat currently duplicates CustomSeparatorsInterval (it sets CustomSeparators but doesn’t configure any label formatting). If this test is meant to cover custom label formatting, add a Labeler/Labeler-like formatter or other formatting-related axis settings so the snapshot actually validates label format behavior.
Adds multiple new tests (~60) and a new way to test things in the library.
This adds new possibilities to tests features that are complex to tests, it compares a committed image in the repo (expected result) with the generated image by the test (new result); If both are similar (defined by a threshold) then the tests pass. This means that when somethings need to change in the library, a new image must explicitly commit the new image.