Fixed "/Users/runner/work/SharpSDL3/SharpSDL3/tests/SharpSDL3.Tests/N… - #17
Conversation
…ativeSurfaceTests.cs(27,87): error CS0117: 'PixelFormat' does not contain a definition for 'Rgb888' [/Users/runner/work/SharpSDL3/SharpSDL3/tests/SharpSDL3.Tests/SharpSDL3.Tests.csproj]"; changed to Bgra8888 for testing
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdates a failing surface format unit test to use a valid pixel format enum value supported by the current SDL/SharpSDL3 API. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The test description suggests validating multiple formats including the one that triggered CS0117; instead of silently swapping
Rgb888forBgra8888, consider either conditionally skipping the unsupported format or updating the test name/intent to reflect that only 32‑bit BGRA/RGBA/ARGB formats are covered. - If the underlying goal is to ensure coverage of a non‑alpha format, this change shifts the test to three alpha-inclusive formats; consider whether a different supported format better preserves the original intent or add a brief comment explaining why
Bgra8888was chosen.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The test description suggests validating multiple formats including the one that triggered CS0117; instead of silently swapping `Rgb888` for `Bgra8888`, consider either conditionally skipping the unsupported format or updating the test name/intent to reflect that only 32‑bit BGRA/RGBA/ARGB formats are covered.
- If the underlying goal is to ensure coverage of a non‑alpha format, this change shifts the test to three alpha-inclusive formats; consider whether a different supported format better preserves the original intent or add a brief comment explaining why `Bgra8888` was chosen.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
Updates the surface format coverage in NativeSurfaceTests to fix a build error caused by referencing a non-existent PixelFormat.Rgb888 enum value.
Changes:
- Replaces
PixelFormat.Rgb888withPixelFormat.Bgra8888inSurface_CreateAndDestroy_MultipleFormats().
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Adonis Deliannis <blizzardo1@blizzeta.net>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| if (!RequireSdl()) return; | ||
| var formats = new[] { PixelFormat.Rgba8888, PixelFormat.Argb8888, PixelFormat.Rgb888 }; | ||
| var formats = new[] { PixelFormat.Rgba8888, PixelFormat.Argb8888, PixelFormat.Xrgb8888 }; |
There was a problem hiding this comment.
PR description/Sourcery summary mentions switching the test to PixelFormat.Bgra8888, but the code now uses PixelFormat.Xrgb8888. Please update the PR description to match the actual change, or switch the test to the intended format. (If you were aiming for the old “Rgb888” 24-bit equivalent, this binding already defines PixelFormat.Rgb24 in SDL3/Enums/PixelFormat.cs.)
…ativeSurfaceTests.cs(27,87): error CS0117: 'PixelFormat' does not contain a definition for 'Rgb888' [/Users/runner/work/SharpSDL3/SharpSDL3/tests/SharpSDL3.Tests/SharpSDL3.Tests.csproj]"; changed to Bgra8888 for testing
Summary by Sourcery
Update surface format test to use a valid pixel format constant.
Bug Fixes:
Tests: