fix(tests): finish the public build before closing the popup; widen the messages loading-state wait (release-1.12.0) - #14596
Merged
Conversation
…he messages loading-state wait Nightly 31907290063 (main @ b40b405) failed exactly two Playwright shards. Windows 24/70 - messages.a11y "scans the named loading state": the expect after `page.goto("/settings/messages")` used the default 5s. The trace shows goto returning at `load`, then auto_login (1.6-3.0s) -> whoami -> config -> the lazy settings route; the messages query mounted 7.0s / 7.5s after goto, 1.4s / 1.7s after the expect gave up. The aria snapshot at failure was the app-level "Loading..." page, not SessionView's status. Use TIMEOUTS.standard, which the identical held-response loading scan in knowledge-bases.a11y already uses. Linux 41/70 - publish-flow: the spec sent a message in the shareable playground popup and closed it 30ms later, while the public build was still in flight. Aborting that request mid-write made the backend terminate its aiosqlite connections under cancellation; the trace + backend log show a ~60s window where every SQLite writer stalled (the un-publish PATCH never answered, the retry's auto_login hung 34s+, the sibling worker's build took 71s instead of 0.66s) while reads kept answering in ms. Wait for the build to finish (Stop visible -> hidden via the shared sendPlaygroundMessage helper) before closing the popup, which also proves the published playground completes a run rather than merely starting one. Verified locally against the full Playwright stack: both tests pass. (cherry picked from commit 517765c)
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughThe PR updates two Playwright tests. The accessibility test uses the standard loading timeout. The publish-flow test uses ChangesPlaywright test reliability
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
✅ Test Coverage AdvisorNo source changes detected without accompanying tests. Thanks for keeping coverage up! 🎉
|
github-actions
Bot
added
bug
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
Clean cherry-pick of #14595 (main) onto
release-1.12.0— same commit, no conflicts; both spec files were byte-identical between the branches.Nightly 31907290063 failed exactly two Playwright shards, and the release branch has identical exposure (same
@releasesuite, same SQLAlchemy 2.0.51 / aiosqlite 0.22.1, and #14593 already ported):messages.a11y.spec.ts"scans the named loading state": the expect right afterpage.goto("/settings/messages")used the default 5s; the trace shows the messages page mounting 7.0s / 7.5s after goto (post-loadapp boot:auto_login1.6–3.0s → whoami → config → lazy route). NowTIMEOUTS.standard, matching the identical loading scan inknowledge-bases.a11y.publish-flow.spec.ts: the spec closed the shareable-playground popup 30ms after sending, mid-build. Aborting that request mid-write left the backend with a ~60s window where every SQLite writer stalled while reads answered in ms (un-publish PATCH never answered; retry'sauto_loginhung 34s+; sibling worker's build took 71s vs 0.66s). Now runs the send throughsendPlaygroundMessage(newPage, "Hello", { surface: "shareable" }), which waits for the Stop button to appear and clear before the popup closes — and asserts the published playground actually completes a run.Full trace/backend-log analysis in #14595.
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit