fix(tests): stop swallowing strict-mode violations across the acceptance suite - #5855
Merged
Merged
Conversation
global-setup.ts's 27 isVisible/isEditable/isChecked/getAttribute readiness checks all used the raw `.catch(() => false)` pattern PR 5824 identified: a strict-mode violation (a locator matching more than one element) gets silently turned into a permanent `false` instead of failing loudly, so a broken selector reports as a generic auth-flow timeout instead of naming the real cause. Switch every site to pollLocatorState (web/tests/utils/index.ts, added in PR 5824), which still swallows ordinary "not there yet" failures but rethrows a strict-mode violation.
Same conversion as global-setup.ts for the three base/user fixture helper modules (apiHelpers, providerHelpers, authHelpers): 13 more `.catch(() => false)` readiness checks now use pollLocatorState so a locator that starts matching more than one element fails with its real strict-mode message instead of a generic "page never reached a stable ready state" timeout. providerHelpers/index.ts already imported pollLocatorState from PR 5824; this converts the 4 sites in that file the earlier pass left behind.
…ptance tests Converts the remaining 40 `.catch(() => false)` readiness checks in the human-annotation, auto-evaluation, and evaluators acceptance suites to pollLocatorState, same as PR 5824's four sites: ordinary "not there yet" failures still poll normally, but a strict-mode violation now surfaces with its real message instead of a misleading readiness timeout. Left one site unconverted in human-annotation/tests.ts (ensureSingleHumanEvaluatorSelection's evaluateAll check): evaluateAll runs over every matched row regardless of count, so it can never throw a strict-mode violation, and a one-line comment says so.
…cceptance tests Same pollLocatorState conversion for the playground, observability, app creation, deployment, use-api, and settings/model-hub acceptance suites (16 sites total). deployment/index.ts and use-api/index.ts already imported pollLocatorState from PR 5824's deployVariantToEnv fix; this converts the one remaining site in each of those two files.
Converts the last 3 sites (openInviteMembersModal x2, dismissInvitedUserLinkDialog) to pollLocatorState.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
dosubot
Bot
added
size:XS
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change replaces direct Playwright visibility and checked-state checks with ChangesPlaywright acceptance flows
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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 |
5 tasks
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
1 participant
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.
Summary
PR 5824 fixed four sites where
LOCATOR.isX().catch(() => false)silently turned a Playwright strict-mode violation (a locator matching more than one element) into a permanentfalse. A poll loop built on that pattern can't tell "not ready yet" from "the selector itself is broken" — it just times out with a message that names no real cause (e.g. "page never reached a stable ready state"), and someone spends hours looking in the wrong place. That PR addedpollLocatorState(web/tests/utils/index.ts) as the fix and the pattern to copy.105 more sites used the raw pattern (102 literal matches of
.catch(() => false)across 16 files; one of those matches was a comment mention, not real code — 101 real sites). This converts all but one of them.What changed, and what didn't
Converted 101 sites across 16 files to
pollLocatorState, in 5 reviewable commits:global-setup.ts(27 sites)apiHelpers,providerHelpers,authHelpers(13 sites)human-annotation,auto-evaluation,evaluators(40 sites)playground,observability,app,deployment,use-api,settings/model-hub(16 sites)members(3 sites)Left exactly one site unconverted, with a comment explaining why:
human-annotation/tests.ts'sensureSingleHumanEvaluatorSelectioncalls.evaluateAll(...)on a locator, not.isVisible()/.isEnabled()/etc.evaluateAllruns over every element the locator currently matches, regardless of count — it can never throw a strict-mode violation, sopollLocatorStatewould be a no-op there. I checked every other site individually for a similar "deliberate swallow" case; this was the only one.Verification — before/after against a real failure
A passing suite doesn't prove the messages improved (that's the whole point PR 5824 made). I reproduced a real strict-mode violation live against the deployed EE dev stack (not a fabricated error string) and ran both patterns against it.
The originally-documented duplicate ("Add endpoint" rendered twice — header + empty state, per
providerHelpers/index.ts's existing comment) no longer reproduces on the account I had a live session for (it already has a custom provider configured from an earlier run, so the empty-state row isn't showing). I demonstrated the identical mechanism against a locator that's genuinely ambiguous on that same live page today: the Model Hub's "Configure now" button, which renders once per standard provider row (13 real matches: OpenAI, Mistral, Cohere, Anthropic, ...).Before (
LOCATOR.isEnabled().catch(() => false)):The caller sees only
false. A poll loop built on this reports something like "Models page never reached a stable ready state" — naming no real cause, exactly the failure mode PR 5824 described.After (
pollLocatorState(() => LOCATOR.isEnabled())):The real Playwright error — naming the ambiguous selector and listing the matched elements — reaches the test instead of being swallowed.
Test plan
eslint --fixon all 16 touched files — no new errors (verified every reported error pre-exists on an untouched line viagit diff)playwright test --listloads all 80 OSS + 67 EE acceptance tests with the converted code (no import/syntax breakage)