fix(#404): cover every locale tag the sanitizer handles, and what it leaves behind - #406
Open
Matobi98 wants to merge 1 commit into
Open
fix(#404): cover every locale tag the sanitizer handles, and what it leaves behind#406Matobi98 wants to merge 1 commit into
Matobi98 wants to merge 1 commit into
Conversation
… what it leaves behind
Contributor
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
fix(#404): cover every locale tag the sanitizer handles, and what it leaves behind
Closes #404. Follow-up to #370, from @grunch's third finding there.
No app code changes:
lib/,web/andrust/are untouched, and the bundle that ships is byte-identical to main's. What changes is what CI checks before letting a change through.The gap
The locale guard added in #370 only ever exercised
C. Playwright'slocaleoption normalizes the tag before the page sees it —en_USarrives as a validen-US,""falls back to the system locale — so two of the three tags named in #227's acceptance criterion were handled by the sanitizer but never reached it in CI. The mixed list (["C","es-AR"]→["es-AR"]) was tested by nothing at all.That is a limit of that one Playwright option, not of the browser.
addInitScriptruns inside the page before any of its own scripts, so it can hand the engine a tag Playwright would never deliver.What this adds
SMOKE_NAVIGATOR_LANGUAGESshadowsnavigator.language(s)from an init script. Comma-separated, used verbatim, unset means "do not touch" — so every existing run is unaffected.!== undefinedrather than a truthiness check, because the empty string is one of the broken tags.configurable: trueis load-bearing: the sanitizer bails out when either property is locked down, so a non-configurable shadow would make it skip the very path under test and pass for the wrong reason.SMOKE_EXPECT_LANGUAGESasserts whatnavigator.languagesreads after the sanitizer ran. This is what makes the mixed list worth running — see below.The matrix in
web-build.ymlrunsC,en_US,""andC,es-AR, each against the shipped bundle (must pass, with the expected locale) and against the control bundle (must fail with a locale error).The control bundle is now built once in its own step instead of being rebuilt inside the control, since two places would otherwise duplicate the strip and drift. The "did the strip actually happen" check moved with it: if the marker is ever renamed, the cut is a no-op and every control silently compares the bundle against itself.
The
SMOKE_LOCALE=Cstep and its control stay. It is the one path where the tag reaches the page as the browser's own locale rather than as a shadow this repo installed.Two things this turned up
1. The empty tag crashes with a different message. Three of the four controls die with
Incorrect locale information provided; the empty one dies withFirst argument to Intl.Locale constructor can't be empty or missing. Both are the engine refusing a locale beforerunApp, but the control introduced in #370 required the first string exactly — so the empty case would have reported "the control failed, but not with the locale RangeError", a false negative on a control that worked perfectly.That check is not wrong today: it only ever runs
C, and forCthe signature is right. It becomes too narrow the moment this PR starts feeding it the other tags, so widening it belongs here rather than in a follow-up. The accepted signatures are now a job-levelLOCALE_CRASH_SIGNATURE, kept exact rather than loosened to something likelocale— a lax pattern is what lets a control celebrate an unrelated crash.2. "The view mounted" cannot judge the mixed list. With
C,es-AR, a sanitizer that keepses-ARand one that drops the whole list for the fallback both boot perfectly; only the second silently costs the user their language. Without reading the result back, that case tests nothingCdoes not already test.Hence
SMOKE_EXPECT_LANGUAGES. Mutation-checked: patching the sanitizer to discard the whole list whenever any tag is invalid leavesC,en_USand""green and fails only the mixed case, withnavigator.languages is "en-US", expected "es-AR".Verified
Against a release bundle built on this branch (
build-web.sh --release+flutter build web --release, Flutter 3.38.2), running the matrix exactly as the workflow does:Cen-USen_USen-US""en-USC,es-ARes-AREight for eight. Plus
pages_bundle_test.dart17/17 andselftest.mjs3/3, both of which cover this workflow and this script.Cost
Four extra control runs at ~20s each — each waits out
SMOKE_TIMEOUT_MSfor a view that will never mount — so roughly 80s added per PR. The positive runs are ~1s each. That is the price of the guard being self-validating; the alternative is a matrix that can go green for the wrong reason.For #227
Its second acceptance criterion carries a caveat that CI only reaches
C. Once this lands that no longer holds and the caveat can go — happy to edit it, or leave it to whoever owns the issue.