More tests - #2096
Conversation
|
Thanks for your contribution! The build and test process is starting. This may take a while. All packages have been packed successfully! 📦✅ The packages will be available for 30 days, you can either use them directly, or wait for this PR to be merged to have them published to NuGet.org. Tests will start now, you can monitor their progress below or at the actions tab. Test Results Summary (Failure) ❌ 😥
0 skipped. |
There was a problem hiding this comment.
Pull request overview
This PR adds GPU/hardware acceleration tests and improves test organization. It introduces a new RendererName property to CoreMotionCanvas to enable testing of different rendering modes, adds tests for both CPU and GPU rendering in PieChartTests and CartesianChartTests, and consolidates Avalonia-specific tests by moving the virtualization test from a dedicated AvaloniaTests.cs file into CartesianChartTests.cs.
Changes:
- Added
RendererNameproperty toCoreMotionCanvasfor renderer identification - Added GPU rendering tests (
ShouldLoadHardwareAcceleratedView) to verify hardware acceleration works correctly - Enhanced existing tests to verify CPU rendering by default
- Consolidated Avalonia virtualization test into
CartesianChartTests.csand removedAvaloniaTests.cs - Updated build configuration comments to clarify GPU/VSYNC settings
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/SharedUITests/PieChartTests.cs | Added GPU rendering test and CPU rendering verification to existing test |
| tests/SharedUITests/CartesianChartTests.cs | Added GPU rendering test, CPU rendering verification, and moved Avalonia virtualization test |
| tests/SharedUITests/AvaloniaTests.cs | Removed file - test moved to CartesianChartTests.cs |
| src/LiveChartsCore/Motion/CoreMotionCanvas.cs | Added public RendererName property to expose renderer information |
| Directory.Build.props | Updated comments to clarify GPU configuration options |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var sut = await App.NavigateTo<Samples.General.FirstChart.View>(); | ||
| await sut.Chart.WaitUntilChartRenders(); | ||
|
|
||
| Assert.DoesNotContain(sut.Chart.CoreCanvas.RendererName, "GPU"); |
There was a problem hiding this comment.
This assertion assumes that GPU rendering is not enabled by default. However, if the ShouldLoadHardwareAcceleratedView test runs before this test and fails to restore settings, this assertion could fail. Consider making this test more resilient by explicitly setting the GPU mode to false at the start of the test, or ensuring proper test isolation.
|
Thanks for your contribution! The build and test process is starting. This may take a while. All packages have been packed successfully! 📦✅ The packages will be available for 30 days, you can either use them directly, or wait for this PR to be merged to have them published to NuGet.org. Tests will start now, you can monitor their progress below or at the actions tab. Test Results Summary (Failure) ❌ 😥
0 skipped. |
|
Thanks for your contribution! The build and test process is starting. This may take a while. All packages have been packed successfully! 📦✅ The packages will be available for 30 days, you can either use them directly, or wait for this PR to be merged to have them published to NuGet.org. Tests will start now, you can monitor their progress below or at the actions tab. Test Results Summary (Failure) ❌ 😥
0 skipped. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var sut = await App.NavigateTo<Samples.Pies.Basic.View>(); | ||
| await sut.Chart.WaitUntilChartRenders(); | ||
|
|
||
| Assert.Contains("GPU", sut.Chart.CoreCanvas.RendererName); | ||
| Assert.ChartIsLoaded(sut.Chart); | ||
|
|
||
| // restore default settings for other tests | ||
| LiveChartsCore.LiveCharts.Configure(config => config.HasRenderingSettings(builder => builder.UseGPU = false)); |
There was a problem hiding this comment.
The test modifies global configuration (LiveCharts.Configure) and relies on cleanup code at the end. If the test fails or is interrupted before line 74, the GPU setting will remain enabled, potentially affecting subsequent tests. Consider using a try-finally block to ensure the configuration is always restored, or implement IDisposable/IAsyncLifetime for proper test cleanup.
| var sut = await App.NavigateTo<Samples.Pies.Basic.View>(); | |
| await sut.Chart.WaitUntilChartRenders(); | |
| Assert.Contains("GPU", sut.Chart.CoreCanvas.RendererName); | |
| Assert.ChartIsLoaded(sut.Chart); | |
| // restore default settings for other tests | |
| LiveChartsCore.LiveCharts.Configure(config => config.HasRenderingSettings(builder => builder.UseGPU = false)); | |
| try | |
| { | |
| var sut = await App.NavigateTo<Samples.Pies.Basic.View>(); | |
| await sut.Chart.WaitUntilChartRenders(); | |
| Assert.Contains("GPU", sut.Chart.CoreCanvas.RendererName); | |
| Assert.ChartIsLoaded(sut.Chart); | |
| } | |
| finally | |
| { | |
| // restore default settings for other tests | |
| LiveChartsCore.LiveCharts.Configure(config => config.HasRenderingSettings(builder => builder.UseGPU = false)); | |
| } |
| var sut = await App.NavigateTo<Samples.General.FirstChart.View>(); | ||
| await sut.Chart.WaitUntilChartRenders(); | ||
|
|
||
| Assert.Contains("GPU", sut.Chart.CoreCanvas.RendererName); | ||
| Assert.ChartIsLoaded(sut.Chart); | ||
|
|
||
| // restore default settings for other tests | ||
| LiveChartsCore.LiveCharts.Configure(config => config.HasRenderingSettings(builder => builder.UseGPU = false)); |
There was a problem hiding this comment.
The test modifies global configuration (LiveCharts.Configure) and relies on cleanup code at the end. If the test fails or is interrupted before line 120, the GPU setting will remain enabled, potentially affecting subsequent tests. Consider using a try-finally block to ensure the configuration is always restored, or implement IDisposable/IAsyncLifetime for proper test cleanup.
| var sut = await App.NavigateTo<Samples.General.FirstChart.View>(); | |
| await sut.Chart.WaitUntilChartRenders(); | |
| Assert.Contains("GPU", sut.Chart.CoreCanvas.RendererName); | |
| Assert.ChartIsLoaded(sut.Chart); | |
| // restore default settings for other tests | |
| LiveChartsCore.LiveCharts.Configure(config => config.HasRenderingSettings(builder => builder.UseGPU = false)); | |
| try | |
| { | |
| var sut = await App.NavigateTo<Samples.General.FirstChart.View>(); | |
| await sut.Chart.WaitUntilChartRenders(); | |
| Assert.Contains("GPU", sut.Chart.CoreCanvas.RendererName); | |
| Assert.ChartIsLoaded(sut.Chart); | |
| } | |
| finally | |
| { | |
| // restore default settings for other tests | |
| LiveChartsCore.LiveCharts.Configure(config => config.HasRenderingSettings(builder => builder.UseGPU = false)); | |
| } |
|
|
||
| <!-- | ||
| used to test the library on multiple render modes, | ||
| used to test the library on multiple render modes, this config overrides the the settings defined by the user. |
There was a problem hiding this comment.
The phrase "overrides the the settings" contains a duplicate word "the". It should be "overrides the settings".
| used to test the library on multiple render modes, this config overrides the the settings defined by the user. | |
| used to test the library on multiple render modes, this config overrides the settings defined by the user. |
|
Thanks for your contribution! The build and test process is starting. This may take a while. All packages have been packed successfully! 📦✅ The packages will be available for 30 days, you can either use them directly, or wait for this PR to be merged to have them published to NuGet.org. Tests will start now, you can monitor their progress below or at the actions tab. Test Results Summary (Failure) ❌ 😥
0 skipped. |
|
Thanks for your contribution! The build and test process is starting. This may take a while. All packages have been packed successfully! 📦✅ The packages will be available for 30 days, you can either use them directly, or wait for this PR to be merged to have them published to NuGet.org. Tests will start now, you can monitor their progress below or at the actions tab. Test Results Summary (Failure) ❌ 😥
0 skipped. |
This reverts commit 08ebf40.
|
Thanks for your contribution! The build and test process is starting. This may take a while. All packages have been packed successfully! 📦✅ The packages will be available for 30 days, you can either use them directly, or wait for this PR to be merged to have them published to NuGet.org. Tests will start now, you can monitor their progress below or at the actions tab. Test Results Summary (Passed) ✅ 🥳0 skipped. |
|
Thanks for your contribution! The build and test process is starting. This may take a while. All packages have been packed successfully! 📦✅ The packages will be available for 30 days, you can either use them directly, or wait for this PR to be merged to have them published to NuGet.org. Tests will start now, you can monitor their progress below or at the actions tab. Test Results Summary (Failure) ❌ 😥
0 skipped. |
|
@copilot can you implement |
|
@beto-rodriguez I've opened a new pull request, #2097, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
Thanks for your contribution! The build and test process is starting. This may take a while. All packages have been packed successfully! 📦✅ The packages will be available for 30 days, you can either use them directly, or wait for this PR to be merged to have them published to NuGet.org. Tests will start now, you can monitor their progress below or at the actions tab. Test Results Summary (Failure) ❌ 😥
0 skipped. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (4)
src/LiveChartsCore/Motion/CoreMotionCanvas.cs:183
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (var geometry in task.GetGeometries(this))
{
if (geometry is null) continue;
if (DisableAnimations) geometry.CompleteTransition(null);
geometry.IsValid = true;
if (!task.IsPaused)
{
context.ActiveOpacity = geometry.Opacity;
context.Draw(geometry);
}
isValid = isValid && geometry.IsValid;
if (geometry.IsValid && geometry.RemoveOnCompleted)
toRemoveGeometries.Add(
new Tuple<Paint, IDrawnElement>(task, geometry));
}
src/LiveChartsCore/Motion/CoreMotionCanvas.cs:339
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (var zone in Zones)
if (zone.RemoveTask(task))
break;
src/LiveChartsCore/Motion/CoreMotionCanvas.cs:387
- This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
foreach (var zone in Zones)
if (zone.ContainsTask(task))
return true;
src/LiveChartsCore/Motion/CoreMotionCanvas.cs:360
- This assignment to geometry is useless, since its value is never read.
foreach (var geometry in task.GetGeometries(this))
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <PropertyGroup Condition="$(TestBuildTargetFramework) == 'net10.0-windows'"> | ||
| <!-- | ||
| workaround, not sure why hardware accelerated views is failing on net10.0-windows10.0.19041.0 in CI environments | ||
| im not able top reproduce the issue locally, ill assume for now that it is a false positive and just exclude that | ||
| hardware accelerated tests in this target. | ||
| --> | ||
| <DefineConstants>$(DefineConstants);TEST_HA_VIEWS</DefineConstants> |
There was a problem hiding this comment.
The logic in this conditional compilation symbol definition appears inverted. The comment says the hardware accelerated views are failing on net10.0-windows in CI, so tests should be excluded. However, the code defines TEST_HA_VIEWS (suggesting tests should be included) when the target framework is net10.0-windows. This appears to be backwards - if tests are failing, the symbol should NOT be defined, or the conditional checks should use a different symbol like SKIP_HA_VIEWS.
| <PropertyGroup Condition="$(TestBuildTargetFramework) == 'net10.0-windows'"> | ||
| <!-- | ||
| workaround, not sure why hardware accelerated views is failing on net10.0-windows10.0.19041.0 in CI environments | ||
| im not able top reproduce the issue locally, ill assume for now that it is a false positive and just exclude that |
There was a problem hiding this comment.
There are grammatical issues in this comment: "im" should be "I'm" and "ill" should be "I'll".
| im not able top reproduce the issue locally, ill assume for now that it is a false positive and just exclude that | |
| I'm not able to reproduce the issue locally, I'll assume for now that it is a false positive and just exclude that |
| test-id: ${{ matrix.id }} | ||
| workloads: ${{ matrix.workloads }} | ||
| # target-framework: ${{ matrix.tf }} | ||
| target-framework: ${{ matrix.tf }} |
There was a problem hiding this comment.
The target-framework parameter is now being passed for all test-browser matrix entries, but the avalonia-browser entry doesn't define a 'tf' value in its matrix configuration. This means target-framework will be passed as an empty string to the run-tests action. Verify that the run-tests action handles empty or undefined target-framework values correctly, or add a tf value to the avalonia-browser matrix entry if it needs one.
| include: | ||
| # - id: avalonia-ios | ||
| # workloads: ios | ||
| - id: avalonia-ios |
There was a problem hiding this comment.
The avalonia-ios test has been re-enabled but doesn't define a 'tf' (target framework) value in its matrix configuration, while the run-tests action is called with target-framework: ${{ matrix.tf }} on line 319. This means an empty string will be passed for target-framework. Verify that the run-tests action handles empty target-framework values correctly, or add a tf value to the avalonia-ios matrix entry if required.
| - id: avalonia-ios | |
| - id: avalonia-ios | |
| tf: net10.0-ios |
|
|
||
| <!-- | ||
| used to test the library on multiple render modes, | ||
| used to test the library on multiple render modes, this config overrides the the settings defined by the user. |
There was a problem hiding this comment.
There is a typo in the comment: "the the" should be just "the".
| used to test the library on multiple render modes, this config overrides the the settings defined by the user. | |
| used to test the library on multiple render modes, this config overrides the settings defined by the user. |
No description provided.