chore: enable Alttester visual tests - #8628
Conversation
…sual tests determinisim
|
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. |
…unity-explorer into chore/visual-test-app-args
pravusjif
changed the title
…re/visual-test-app-args
pravusjif
added
force-build
…vided FullScreenWindow is a borderless window sized to the OS desktop, so the Unity --resolution override is silently clamped to the desktop size. On headless Windows CI (1024x768 default) the framebuffer never reaches the requested WxH, breaking visual-regression captures. Switch to ExclusiveFullScreen whenever the caller explicitly passes a resolution override in fullscreen mode — this triggers a real DXGI/display mode-switch and the captured framebuffer matches the requested size. Behavior without --resolution is unchanged.
… chore/visual-test-app-args
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Claude finished @pravusjif's task in 2m 11s —— View job PR Review (sync — latest HEAD
|
| Item | Before | After |
|---|---|---|
BootstraperShould.cs:16 — async void Setup() |
async void with UniTask.WaitUntil workaround in each test |
async Task Setup() — workarounds removed |
AlttesterSceneReadinessProbe.cs:57 — string concat allocation |
parcel.x + "," + parcel.y |
StringBuilder(16).Append(parcel.x).Append(',').Append(parcel.y) |
Both fixes are correct and clean.
No blocking issues found
All changed files reviewed:
AlttesterSceneReadinessProbe.cs—volatilefield correct for single-writer/multi-reader pattern.internalSet/Clear,publicquery methods. Hand-rolled JSON builder is appropriate here (no serializer dep). All gated by#if ALTTESTER.SceneFacade.cs—ClearIfCurrentcalled in bothDispose(beforeDisposeInternal) andDisposeAsync(before the main-thread switch).SetIsCurrentcorrectly branches onisCurrent. All three sites properly#if ALTTESTER-gated.SkyboxPlugin.cs—SetInitialTimelocal function now called unconditionally, pins to noon when!timeEnabledso skybox is deterministic regardless of host clock.SkyboxTimeUpdateSysteminjection still gated byskyboxTimeEnabled. Correct.SavedQualitySettingsApplier.cs/QualitySettingsController.cs—--graphicsoverride applied without persisting (persist: false).Customrejected withLogWarning. Clean.NativeWindowManager.cs— Three-way fullscreen decision logic is correct:--windowed-mode→ forced false; saved pref exists → from pref; else → inherit Unity's mode.ResolveFullScreenModehelper andExclusiveFullScreen/Windows branch are clean.MainSceneLoader.cs—--skip-minimum-specs-screencorrectly yields to--force(&& !forceShow). No issue.BootstraperShould.cs—async Task Setup()with remainingUniTask.Yield()is correct; ensures substitutes are ready before tests run.AppArgsFlags.cs— Three new string constants (SKIP_MINIMUM_SPECS_SCREEN,GRAPHICS,DISABLE_HUD). No issues.
Non-blocking warning (carried from prior review)
W.7a — visual-regression.yml:162: reusable workflow pinned to @main with secrets: inherit
uses: decentraland/explorer-automation/.github/workflows/run-visual-suite.yml@main with secrets: inherit. SHA-pin recommended before this workflow is used as a template. (Cannot be modified in this context — GitHub App lacks .github/workflows/ write permission.)
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Touches scene runtime (SceneFacade lifecycle with volatile cross-thread state), skybox plugin, quality settings controller, native window management, async patterns (UniTask), and a new CI/CD workflow.
QA_REQUIRED: YES
· branch chore/visual-test-app-args
|
Claude finished @pravusjif's task in 1m 4s —— View job Claude PR Security Re-Review — sync
What changed since the prior security reviewOne new commit on top of
The two changes:
Verified:
Prior findings — current status
None of these are reachable from the new commit; they are properties of the workflow file, which is unchanged in this sync. VerdictThe new PR-side commit only addresses two non-blocking C# style comments from the previous review — no security-relevant changes. The prior verdict carries over unchanged: W.7a is the one finding worth resolving before merge (SHA-pin the DEPENDENCY_REVIEW: NEEDS_ATTENTION (carried over from the prior six reviews — no new findings on this sync) |
|
PR #8628, run #26196359619 Builds: Windows change, Windows baseline, macOS change, macOS baseline Framework 13 i7
|
github-project-automation
Bot
moved this from With QA / Awaiting Review
to Done
in Explorer Alpha
4 participants
A series of small changes and additions to enable the external Alttester automation tests the maximum determinism for VISUAL TESTS (automation tests like at https://github.com/decentraland/explorer-automation)
--disable-hudapp arg flag needed for visual tests determinism (Explorer HUD , NOT scenes SDK UI).--graphicsapp arg flag needed for visual tests determinism.--skip-minimum-specs-screenapp arg flag needed for visual tests determinism.--skybox-time-enabledflagReminder: the
ALTTESTERcompiler definition is only enabled when the--alttesterapp arg is used, Metaforge already takes care of that when opening a build to run the Alttester tests.Added Scene Readiness Probe for AltTester Visual Tests
What
Adds a tiny, AltTester-queryable surface that reports whether the player's current scene has finished loading. Replaces the heuristic "wait for a pixel-stable frame" used by the visual test fixtures, which silently passed on stale frames during hot-reload.
How it works
SceneFacade.SetIsCurrent(true)registers the facade on the probe;SetIsCurrent(false)/Dispose/DisposeAsyncclear it.IsCurrentSceneReady()returnscurrentFacade?.IsSceneReady()— the sameSceneLoadingConcludedcheck already used byUnloadSceneLODSystem,ReloadSceneChatCommand, etc.explorer-automation) callsAltDriver.CallStaticMethod<bool>("SceneRunner.Scene.AlttesterSceneReadinessProbe", "IsCurrentSceneReady", "SceneRunner.Scene", …)and polls until ready.Why static
AltTester's
CallStaticMethodis the only reflection path that doesn't require first locating a MonoBehaviour on a GameObject. A non-static alternative would mean (a) a sink interface injected into everySceneFacadeinstance through the DI chain (SceneFactoryand its parents), (b) a no-op implementation for non-AltTester contexts, and (c) updating every test that builds a facade by hand. That's a meaningful refactor for a hook that exists purely to serve AltTester. Static + compile-time gating delivers the same effective footprint with a tenth of the surface area.Gating
Everything is wrapped in
#if ALTTESTER:AlttesterSceneReadinessProbe.csbody.SceneFacade.cs.The
ALTTESTERdefine is set inProjectSettings.assetunder Standalone defines for Editor and local builds, and stripped byCloudBuild.cswhenIS_RELEASE_BUILD=true. Release builds shipped to end-users contain no probe class, no fields, no call sites — zero footprint.TEST INSTRUCTIONS (MacOS ONLY)
Clone the automation tests repo: https://github.com/decentraland/explorer-automation and step at the root of that folder, the following commands have to be ran in there -> Run the visual tests locally and confirm both the regression tests pass:
make installmetaforge account createmetaforge account login(select one of the existent accounts)metaforge explorer server startmetaforge explorer test chore/visual-test-app-args --filter "Category=Visual"[REPORT SHOULD BE GREEN, ALL TESTS PASS]metaforge explorer server stop