Playground validation: once-only failure cleanup and pixel-count wording - #1863
Merged
bkaradzic-microsoft merged 1 commit intoSep 3, 2026
Conversation
Wrap each test's done() so stopRenderLoop + inter-test cleanup run once before the next test (render-loop throws, onReadyTimeout, and load/eval errors previously bypassed evaluateScreenshot cleanup). Soften the "identical pixel count means regression" guidance per post-merge review on BabylonJS#1838. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d47cbab2-d751-4cf9-984f-4412dd9ec601
bkaradzic-microsoft
requested review from
bghgary
and
a lite review from Copilot
September 3, 2026 00:06
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The changes are cohesive, tool-verified within the updated harness flow, and do not introduce any clear correctness issues while improving test isolation and completion robustness.
Pull request overview
Follow-up improvements to the native Playground validation harness to ensure each test completes exactly once and reliably resets engine/scene state between tests, preventing cascading failures or stale render-loop callbacks from affecting subsequent tests.
Changes:
- Centralized inter-test teardown into
cleanupAfterTest()and routed all completion paths through a once-only wrappermakeTestDone(). - Ensured
runTest()always uses the once-only completion wrapper and returns immediately on out-of-range indices. - Updated pixel-diff diagnostics wording to clarify what a stable pixel-difference count does and does not prove.
File summaries
| File | Description |
|---|---|
| Apps/Playground/Scripts/validation_native.js | Adds once-only test completion wrapper plus shared cleanup to prevent stale render-loop callbacks and improve failure diagnostics wording. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
bkaradzic-microsoft
added a commit
to bkaradzic-microsoft/BabylonNative
that referenced
this pull request
Sep 4, 2026
Keeps shotgun JsRuntimeHost/SPIRV wiring and existing FlipFragCoordY, PCF sampler-flip, compute Program::InitializeCompute, and inter-stage varying location assignment. Drops the duplicate FragCoord uniform upload introduced by the auto-merge of BabylonJS#1840. Takes upstream Playground validation cleanup (BabylonJS#1863) and shader-info cache bits. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
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.
Summary
Post-merge follow-up to #1838 (review):
catch,onReadyTimeout, snippet/script load/eval errors) now goes throughmakeTestDone:stopRenderLoop(), sharedcleanupAfterTest()(scene/strays, stencil/scissor, effects, texture cache, SceneLoader plugin observable), then the outer callback. Prevents stalerunRenderLoopcallbacks from rendering the next scene or callingdone(false)repeatedly under continue-on-failure.Test plan