feat: expose feature flag state to AltTester tests - #9682
Conversation
AlttesterFeatureFlagsProbe reads FeatureFlagsConfiguration and FeaturesRegistry through AltDriver.CallStaticMethod, so the UI suite can assert against the flags the client gates on instead of re-fetching the remote document and re-deriving Unleash evaluation. ALTTESTER-gated, like AlttesterSceneReadinessProbe. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Neither repo mentioned them, so the feature flag probe had nowhere to be discovered from. Lists all three, and points feature-flags.md at it so the next person reaches for the probe instead of re-fetching the document. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review — feat: expose feature flag state to AltTester tests
STEP 2 — Root-cause check ✅
Problem: AltTester UI tests hard-code flag-gated sidebar buttons as always-present. When alfa-marketplace-credits was turned off remotely, the test failed even though the client was correct — the test expectation was wrong.
Does the diff fix the cause? Yes. The root cause is that the test suite cannot access the client's resolved feature flag state (Unleash hostname strategy, app args, and editor overrides make external re-derivation unreliable). This probe exposes the client's actual internal state to the test framework. PASS — this is the correct fix.
STEP 3 — Design & integration ✅
MANDATORY OWNER SEARCH: AlttesterFeatureFlagsProbe is a stateless static class — it holds no fields, no collections, no subscriptions, and no persistent state across frames. It is a read-only accessor, not a lifecycle owner. There is nothing to create, destroy, subscribe, or tear down.
It reads from two existing [Singleton] instances:
FeatureFlagsConfiguration.Instance— created during app startup, lives for the app lifetime. Defined inExplorer/Assets/DCL/FeatureFlags/FeatureFlagsConfiguration.cs.FeaturesRegistry.Instance— created during app startup, depends onFeatureFlagsConfiguration. Defined inExplorer/Assets/DCL/FeatureFlags/FeaturesRegistry.cs.
Neither singleton gains a new lifecycle responsibility from this probe. The probe simply reads their existing public API.
Pattern precedent: Follows the established AlttesterSceneReadinessProbe pattern (Explorer/Assets/DCL/Infrastructure/SceneRunner/Scene/AlttesterSceneReadinessProbe.cs) — also a static class gated by #if ALTTESTER, also exposing internal client state to AltTester. The new probe is actually simpler (no mutable state, vs. the scene probe's volatile ISceneFacade? field).
Assembly placement: Sits in Explorer/Assets/DCL/FeatureFlags/, which compiles into DCL.Network — the same assembly as FeatureFlagsConfiguration and FeaturesRegistry. No new assembly references required. Correct location.
Design checklist:
- Duplicates a lifecycle already owned? ❌ No lifecycle.
- Reconciles every frame? ❌ Called on-demand by AltTester.
- Holds persistent state outside ECS? ❌ Stateless.
- Reaches data through repeated intermediary? ❌ Reads canonical singletons directly.
TEARDOWN / CONSUMPTION TRACE: No subscriptions, event hookups, callbacks, connections, buffers, or measurements. Nothing to tear down.
PASS — design is sound.
STEP 4 — Member audit ✅
| Method | Purpose | Consumers | Verdict |
|---|---|---|---|
IsFlagEnabled(string) |
Raw remote flag state via FeatureFlagsConfiguration |
AltTester suite | Legitimate thin accessor for the AltTester API surface; not a redundant wrapper — it provides the string-based entry point AltTester needs |
IsFeatureEnabled(string) |
Resolved FeatureId state via FeaturesRegistry (flag + app args + editor) |
AltTester suite | Distinct from IsFlagEnabled — reads a different source with different semantics. String→enum parsing is necessary for AltTester's reflection-based CallStaticMethod |
GetFlagVariantJson(string) |
Variant name + payload as JSON | AltTester suite | Exposes data neither boolean method provides (variant name, payload type/value for allowlist gating) |
GetStatusJson() |
Diagnostic snapshot of all enabled flags/features | AltTester suite on failure | Deliberately never throws — called when something is already wrong. Justified for failure diagnostics |
ParseFeatureId(string) (private) |
Case-insensitive FeatureId parse + validation |
IsFeatureEnabled only |
Clean extraction of parse+validate; throws ArgumentException on typos — design intent per PR description |
No failure modes triggered: no single-use merges needed (each method has a distinct AltTester purpose), no absent≠false confusion, no re-derivation, no redundant guards.
STEP 5 — Line-level review ✅
Pass A — Blocking issues: None found.
- Code quality / CLAUDE.md:
Enum.GetValues(typeof(FeatureId))inGetStatusJson()boxes each enum value and allocates an array;AllEnabledFlags(existing code) uses LINQ. Both are acceptable —GetStatusJson()is a diagnostics method called only on test failure, never per-frame. - Bugs / runtime errors:
IsFlagEnabled,IsFeatureEnabled, andGetFlagVariantJsonwill throw if singletons aren't initialized. This is by design — AltTester tests run after client initialization;GetStatusJson()guards with try/catch for the pre-initialization diagnostic case. Consistent with howAlttesterSceneReadinessProbemethods behave. - Security: All code gated by
#if ALTTESTER, stripped from release builds byCloudBuild.cs. No secrets, no auth bypass, no injection vectors. - Performance: Not a hot path — called by AltTester framework, not per-frame.
- Error handling:
GetStatusJson()has proper try/catch for the uninitialized case. Other methods are intentionally fail-fast. - Unused imports: All imports consumed.
- Resource leaks: None — no subscriptions, events, handles, or disposables.
8–11. No issues.
Pass B — Design, encapsulation & resource smells: None found.
- Naming follows the
Alttester*Probeconvention. - XML doc comments describe behavior accurately (especially the "never throws" contract on
GetStatusJson()). - No magic values, no encapsulation violations, no nullable annotation issues.
ParseFeatureIdcorrectly guards against numeric string parsing withEnum.IsDefinedafterTryParse.
Security review: No security issues found. All code is test instrumentation gated by #if ALTTESTER and stripped from release builds. No hardcoded secrets, no user input from untrusted sources (AltTester calls come from the test harness), no sensitive data exposure in production.
STEP 5 — Test coverage ✅
The test file AlttesterFeatureFlagsProbeShould.cs covers:
- Raw flag reads (enabled, disabled, absent) — 4
TestCases - Case-insensitive
FeatureIdparsing — 2TestCases - Registry vs flag distinction (
ReadFeatureStateFromRegistryNotFromFlag) - Throw on unknown name (including empty string and numeric) — 3
TestCases - Variant payload deserialization
- Absent variant handling
- Status snapshot shape with enabled flags/features
- Not-yet-initialized path (singletons not set)
Tests follow AAA pattern, use NUnit + NSubstitute, and properly Reset()/Initialize() singletons in SetUp/TearDown.
STEP 6 — Complexity: SIMPLE
4 files (2 source + 2 .meta), ~280 lines total, no ECS/async/lifecycle changes. Straightforward read-only accessor with comprehensive tests.
STEP 7 — QA: NO
All code gated by #if ALTTESTER, stripped from release builds. No production code paths change. No user-facing behavior affected.
STEP 8 — Warnings
None. Main scene not modified.
STEP 9 — Verdict
Clean, well-structured PR that follows established patterns (AlttesterSceneReadinessProbe), has excellent test coverage, correct assembly placement, and no production impact. No findings.
REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Stateless test-instrumentation probe in FeatureFlags — no ECS, async, lifecycle, or runtime changes
QA_REQUIRED: NO
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
What does this PR change?
Adds
AlttesterFeatureFlagsProbe— a static,ALTTESTER-gated entry point that lets theAltTester UI suite read feature flag state from the running client.
The InWorld navbar test hard-codes six flag-gated sidebar buttons as always-present. When
alfa-marketplace-creditswas turned off, the test started failing on every run — a correctclient, a wrong expectation. The suite can't fix that by fetching the flags document itself:
Unleash evaluates its hostname strategy off the
refererheader (a bare fetch returns 51 flagsinstead of 114) and the resolved state also folds in app args and editor overrides. Reading the
client's own values removes that whole class of drift.
Four methods:
IsFlagEnabled(flagId)alfa-marketplace-creditsIsFeatureEnabled(featureId)FeatureIdstate — flag + app args + editor, what the UI gates onGetFlagVariantJson(flagId)GetStatusJson()IsFeatureEnabledthrows on a name that isn't aFeatureIdmember, so a typo fails loudlyinstead of reading as "off" and letting a test assert the wrong thing quietly.
GetStatusJsonis the opposite — it never throws, because a test calls it when something has already gone wrong.
Sits in
Explorer/Assets/DCL/FeatureFlags/, which compiles intoDCL.Network, so it reachesboth
FeatureFlagsConfigurationandFeaturesRegistrywith no new assembly references. Gatedby
ALTTESTERand stripped from release builds byCloudBuild.cs, same asAlttesterSceneReadinessProbe.Docs:
docs/automation-testing.mdgains a Static Probes section — it documented every other partof the AltTester setup but not the
CallStaticMethodhooks, so all three (scene readiness, featureflags,
PerfSampler) are now listed in one place, withdocs/feature-flags.mdpointing at it.No production code paths change.
Test Instructions
EditMode tests cover the probe:
DCL.FeatureFlags.Tests.AlttesterFeatureFlagsProbeShould(raw flag reads, case-insensitive
FeatureIdparsing, throw-on-unknown-name, variant payload,status shape, and the not-yet-initialized path).
Expected result: all tests pass.
Consumed from the automation suite via
AltDriver.CallStaticMethod, assemblyDCL.Network,type
DCL.FeatureFlags.AlttesterFeatureFlagsProbe:Requires an instrumented (non-release) build — release zips strip the
ALTTESTERdefine.Quality Checklist