chore: expose requestid for cross tests - #8862
Conversation
Cross-stack tests in explorer-automation need the auth URL the client opens in the system browser to drive the Playwright side of the device-pairing flow. The URL is otherwise never logged, never surfaced in the Unity scene, and only readable from the system browser via osascript (CI-hostile). This patch writes the opened URL to a known file at the same moment Application.OpenURL is called, so the Playwright orchestrator can detect the auth flow has begun and navigate its own browser to the same URL. Compile-time gated to #if ALTTESTER so shipping builds have ZERO information-leak surface. Same pattern as data-testid annotations in DCL dapp tests: an explicit test affordance owned by production code, only active in instrumented builds.
Cross-stack Playwright tests navigate to the auth URL themselves (they read auth-url.txt and drive their own Chromium). Letting Application.OpenURL also fire just pops a stale system-browser tab the test ignores — and confuses anyone watching the test run. ALTTESTER builds now only write the URL to disk; production builds retain the full system-browser open path. Compile-time gated, zero behaviour change in shipped binaries.
…l write - switch hardcoded mac-only Library/Application Support path to Application.persistentDataPath - route I/O failures through ReportHub.LogException(ReportCategory.AUTHENTICATION) instead of Debug.LogWarning - trim verbose comment block
|
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. |
|
@claude review |
This comment has been minimized.
This comment has been minimized.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…re/expose-requestid-for-cross-tests
|
@claude review |
This comment has been minimized.
This comment has been minimized.
|
PR #8862, run #26298583791 Builds: Windows change, Windows baseline, macOS change, macOS baseline Framework 13 i7
|
cyaiox
changed the title
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Review — chore: expose requestid for cross tests
Files changed: 1 (+20 -1) · CI: pending (editmode/playmode tests, macOS/Windows builds)
Clean, minimal change. The entire #if ALTTESTER block is stripped from production builds — zero risk to shipped code.
Findings
[P2 — Minor] Uri.EscapeUriString deprecation
Uri.EscapeUriString(url) is deprecated in .NET 5+ because it doesn't escape # and ? characters that may appear in the fragment/query. Unity's runtime may still accept it, but consider switching to constructing a new Uri(url) and calling .AbsoluteUri, or using a targeted escape for the components that need it. Not blocking since the previous code already used EscapeUriString — this PR doesn't introduce the pattern, it just preserves it.
[P2 — Minor] Silent failure path under ALTTESTER
If File.WriteAllText throws, the exception is logged via ReportHub but Application.OpenURL is never called (the #else branch is compile-time excluded). In test builds this is the correct behavior (the test will time out with a clear error from the C# polling assertion), but worth a one-line comment noting the intentional suppression so future readers don't assume it's a bug.
[P2 — Minor] PR title vs. actual change
The PR description itself notes the branch name (chore/expose-requestid-for-cross-tests) is from an earlier scope. The PR exposes the auth URL to disk, not a request ID. Consider updating the title to match the body's suggestion: chore(browser): expose auth URL to disk for cross-stack tests (ALTTESTER).
Security
No security issues found. The file write is local-only, compile-time gated, and uses Application.persistentDataPath (no user-controlled path components). The caught exception is logged through the standard diagnostics pipeline — no sensitive data leakage.
Verdict
APPROVE — No P0 or P1 issues. The change is well-scoped, correctly gated, and follows existing project patterns (ReportHub logging, #if ALTTESTER convention).
Reviewed by Jarvis 🤖 · Requested by Gabriel Díaz via Slack
|
PR #8862, run #26892116315 Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Framework 13 i7
|
This comment has been minimized.
This comment has been minimized.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
pravusjif
added
the
no QA needed
This comment has been minimized.
This comment has been minimized.
cyaiox
added
force-build
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.
cyaiox
added
force-build
Commit cb0c4ae suppressed Application.OpenURL under #if ALTTESTER, but ALTTESTER is a compile define on the whole Standalone build target group, so it is active in the Editor and every non-release standalone build — not just automated test runs. That broke wallet login everywhere: the signature page never opened and LoginAsync hung until timeout. Switch the suppression to the runtime --alttester flag (AppArgsFlags.ALTTESTER, the same gate AltTester automation already uses) by injecting IAppArgs into UnityAppWebBrowser. The system browser now opens for all human-run sessions; cross-stack tests launched with --alttester still get auth-url.txt and no stale tab. The disk-write stays inside #if ALTTESTER, so release binaries remain free of the file-write code.
This comment has been minimized.
This comment has been minimized.
Dismissed: latest Claude review no longer auto-approves this PR.
…re/expose-requestid-for-cross-tests
This comment has been minimized.
This comment has been minimized.
Ludmilafantaniella
left a comment
Ludmilafantaniella
left a comment
There was a problem hiding this comment.
Tested on Windows and Mac. ✅
This change is compile-time gated under #if ALTTESTER so production behavior is untouched. Performed a manual QA regression to confirm no regressions were introduced:
- Log In / Log Out ✅
- Backpack and wearables in world ✅
- Emotes in world and in backpack ✅
- Teleport via map, coordinates, and Jump In ✅
- Chat and multiplayer ✅
- Profile card ✅
- Camera ✅
- Skybox ✅
…ser.cs Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com> Signed-off-by: Gabriel Díaz <git@subsme.slmail.me>
|
Claude finished @cyaiox's task in 1m 2s —— View job Claude finished reviewing ——
Root-Cause AssessmentProblem: Cross-stack Playwright tests needed the auth URL that Unity hands off to the system browser. The system browser was inaccessible in CI, and Fix: The diff correctly addresses the cause by: (1) adding a compile-time gate ( Previous Blocking Issue — ResolvedThe P1 flag from the last review cycle (5-line comment block violating CLAUDE.md "Never write multi-line comment blocks — one short line max") has been addressed in the latest commit ( Remaining IssuesNone. No new blocking issues are introduced. Implementation correctness:
REVIEW_RESULT: PASS ✅ |
…-for-cross-tests # Conflicts: # Explorer/Assets/DCL/NetworkDefinitions/Browser/UnityAppWebBrowser.cs
🚦 CI Status
Build failed! Check the logs to see what went wrong. Warnings not reduced: 12699 => 12699 — remove at least 1 warning to merge. Warnings/errors in files changed by this PR (12)Some Unity tests failed ❌
Failed tests (19)
|
This comment has been minimized.
This comment has been minimized.
|
PR #8862, run #32362270851 Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Intel Core i5
Exception breakdown
Apple M1
|
6 participants
Pull Request Description
What does this PR change?
Adds a small, compile-time-gated test affordance to
UnityAppWebBrowser.OpenUrl(string)so cross-stack Playwright tests inexplorer-automationcan intercept the auth-funnel URL the client would otherwise hand off to the system browser.Problem
Cross-stack tests for the device-pairing / auth flow need the URL the client opens in the system browser to drive the web side. Today:
osascript(CI-hostile, mac-only, fragile).Application.OpenURLfire under test also pops a stale tab the Playwright runner ignores, confusing anyone watching the run.Solution
Under
#if ALTTESTERonly:Path.Combine(Application.persistentDataPath, "auth-url.txt")— Playwright polls and reads it.Application.OpenURLentirely.Production builds are untouched — the entire block is stripped at compile time when
ALTTESTERisn't defined.Implementation notes
#if ALTTESTERpattern used inSceneFacade.cs,AlttesterSceneReadinessProbe.cs, andMainSceneLoader.cs.Application.persistentDataPathso the affordance works on macOS, Windows, and Linux without a hardcoded per-OS path. Matches the convention used byDCLPlayerPrefs,ChatHistoryStorage, andThirdWebLoginService.ReportHub.LogException(e, ReportCategory.AUTHENTICATION)so they participate in the project's diagnostics / Sentry pipeline (perCLAUDE.md—ReportHuboverDebug.Log*).Coordination required
The Playwright consumer (
explorer-automation/web/tests/auth/specs/client-to-web-handoff.spec.ts) must read from Unity'spersistentDataPath, which is platform-specific:~/Library/Application Support/Decentraland/Decentraland Unity Explorer/auth-url.txt%USERPROFILE%\AppData\LocalLow\Decentraland\Decentraland Unity Explorer\auth-url.txt~/.config/unity3d/Decentraland/Decentraland Unity Explorer/auth-url.txtExact folder names come from
companyName/productNameinProjectSettings.asset.Files changed
Explorer/Assets/DCL/NetworkDefinitions/Browser/UnityAppWebBrowser.cs(+21 / -1)Test Instructions
Prerequisites
ALTTESTER).explorer-automationcheckout updated to readauth-url.txtfromApplication.persistentDataPath(see Coordination required above).Test Steps
Smoke — production build (no
ALTTESTER)ALTTESTERdefine.ALTTESTER build (cross-stack test path)
ALTTESTERdefined.auth-url.txtis written underApplication.persistentDataPathcontaining the escaped URL.client-to-web-handoff.spec.tsshould pick up the URL and complete the handoff.Failure-path smoke
ALTTESTERdefined, pointApplication.persistentDataPathat a read-only location (or temporarily mark the file read-only after creation).ReportCategory.AUTHENTICATIONexception is logged viaReportHub; auth flow continues without throwing.Additional Testing Notes
#if ALTTESTERblock (includingusing System.IO;andusing DCL.Diagnostics;) is stripped from a non-ALTTESTER build — quick check viailspy/dotPeekon the produced assembly.chore/expose-requestid-for-cross-tests) is from an earlier scope; this PR does not expose a request ID. Worth renaming the PR title to match: "chore(browser): expose auth URL to disk for cross-stack tests (ALTTESTER)".Quality Checklist
Code Review Reference
Please review our Branch & PR Standards before submitting. It explains the automated review flow, QA/DEV approval requirements, and what each label does — especially useful for first-time contributors.