Playground validation: remove cross-test state leaks from the reused engine - #1831
Closed
bkaradzic-microsoft wants to merge 2 commits into
Closed
Playground validation: remove cross-test state leaks from the reused engine#1831bkaradzic-microsoft wants to merge 2 commits into
bkaradzic-microsoft wants to merge 2 commits into
Conversation
The validation runner exits as soon as a test fails, so a run only ever
reports the first failure and everything after it is left unmeasured. That
makes it impossible to answer "how many of the 720 tests actually pass
today?" without repeatedly excluding whatever failed and re-running.
Add --keep-going (-k): a failing test is recorded and the run continues.
The run still exits non-zero if anything failed, so CI semantics are
unchanged for existing invocations, and the summary now lists every failing
test title instead of just the tally.
Default behaviour is untouched -- without the flag the runner still stops on
the first failure.
Running to completion for the first time immediately exhausted the bgfx
framebuffer pool around the FrameGraph tests, so raise the floor from 512 to
2048. 512 was only ever sufficient because the run stopped early; a complete
uninterrupted sweep keeps allocating well past that point.
Verified with two tests pointed at a mismatched reference image plus one
control:
without --keep-going: ran=1 passed=0 failed=1 exit -1
with --keep-going: ran=3 passed=1 failed=2 exit -1
(both failing titles listed)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
…engine The suite reuses a single engine across all tests, so anything that outlives scene.dispose() silently carries into later tests. The browser harness never sees this because it reloads the page per test. Three such leaks: 1. Stray scenes. A test can leave extra scenes registered on the engine (an async load that created its own scene, a scene whose creation promise resolved after validation). They keep their resources alive. Dispose any scene still registered after the test's own scene is disposed. 2. The engine texture cache. BaseTexture._getFromCache keys on url/noMipmap/ isCube only -- not on the load-time options. A test that leaves a reference behind (e.g. assigning one texture to both scene.environmentTexture and a material's reflectionTexture) keeps its internal texture cached across scene.dispose(), so a later test loading the same URL reuses the *previous* test's texture along with its prefiltering/irradiance settings. Release what is left so each test loads its own textures. 3. SceneLoader.OnPluginActivatedObservable. This is global and outlives the scene. Snippets use it to configure the glTF loader (animationStartMode, compileMaterials, ...) and never unregister, so every later glTF test inherits those settings. Also guard a createScene promise that never resolves. The existing onReadyTimeout safety net lives inside processCurrentScene and only applies once the promise has resolved, so a pending createScene promise hangs the whole suite instead of failing one test. This is a no-op for the currently enabled set: 302 ran / 299 passed / 3 failed both before and after. It removes order-dependence that becomes load-bearing as more tests are enabled. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
bkaradzic-microsoft
requested review from
bghgary
and
a lite review from Copilot
August 12, 2026 18:47
Member
Author
|
Folded into #1830 -- both commits touch the same file and tell one story (make a full sweep possible, then make it order-independent), so reviewing them together is less overhead than a stack. The state-leak commit is unchanged, just carried over as the second commit there. |
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the native Playground validation harness to make long-running, full-suite sweeps more reliable by reducing cross-test state leakage when reusing a single engine across hundreds of tests, and by adding runner support for continuing after failures.
Changes:
- Add
--keep-going/-ksupport (C++ CLI + JS harness) and list failed test titles in the final summary. - Add per-test cleanup in
validation_native.jsto dispose stray scenes, clearSceneLoader.OnPluginActivatedObservable, and aggressively clear cached textures to reduce order-dependent behavior. - Increase the enforced minimum for
BGFX_CONFIG_MAX_FRAME_BUFFERSto prevent framebuffer pool exhaustion during uninterrupted sweeps.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Dependencies/CMakeLists.txt | Raises enforced minimum framebuffer pool size for long native validation runs. |
| Apps/Playground/Win32/App.cpp | Plumbs the new keepGoing option into the JS runtime options object. |
| Apps/Playground/Shared/CommandLine.h | Adds KeepGoing option to the shared CLI options struct. |
| Apps/Playground/Shared/CommandLine.cpp | Implements --keep-going / -k flag parsing and help text. |
| Apps/Playground/Scripts/validation_native.js | Implements keep-going behavior, adds leak cleanup between tests, and adds a createScene promise hang guard. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+258
to
+262
| const leakedTextures = engine.getLoadedTexturesCache(); | ||
| for (let i = leakedTextures.length - 1; i >= 0; --i) { | ||
| engine._releaseTexture(leakedTextures[i]); | ||
| } | ||
| engine.clearInternalTexturesCache(); |
Comment on lines
+462
to
+468
| const createSceneTimeoutId = setTimeout(function () { | ||
| if (sceneSettled) { return; } | ||
| sceneSettled = true; | ||
| console.error("createScene promise for " + test.playgroundId + | ||
| " did not resolve within " + (createSceneTimeoutMs / 1000) + "s."); | ||
| failTest(done); | ||
| }, createSceneTimeoutMs); |
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.
Note
Stacked on #1830 — review that one first. This PR's diff is the single
validation_native.jscommit on top.Problem
The suite reuses one engine across all 720 tests, so anything outliving
scene.dispose()silently carries into later tests. The browser harness never sees this because it reloads the page per test.The three leaks
1. Stray scenes. A test can leave extra scenes registered on the engine — an async load that created its own scene, or a scene whose creation promise resolved after validation finished. They stay registered and keep their resources alive.
2. The engine texture cache.
BaseTexture._getFromCachekeys onurl/noMipmap/isCubeonly — not on load-time options. A test that leaves a reference behind (e.g. assigning one texture to bothscene.environmentTextureand a material'sreflectionTexture) keeps its internal texture cached acrossscene.dispose(). A later test loading the same URL then silently reuses the previous test's texture, along with its prefiltering/irradiance settings.3.
SceneLoader.OnPluginActivatedObservable. Global, outlives the scene. Snippets use it to configure the glTF loader (animationStartMode,compileMaterials, …) and never unregister, so every later glTF test inherits those settings.Also: a hang guard
A
createScenepromise that never resolves currently hangs the entire suite. The existingonReadyTimeoutsafety net lives insideprocessCurrentSceneand only applies after the promise resolves. This converts it to a single failed test.(Caveat documented in-code: this only fires if the JS event loop keeps running. A snippet that blocks the JS thread natively isn't rescued by it.)
Impact — deliberately none today
Same three failures, identical set. This PR changes no current result. Its value is removing order-dependence that becomes load-bearing as more tests get enabled — leak #2 in particular produces failures that only reproduce in sequence and pass in isolation, which is exactly the class of bug that's most expensive to debug later.
I'm raising it now rather than bundled with a test-enablement PR so the mechanism can be reviewed on its own.