test(e2e): correct the Cloudflare skip-comment characterizations#1327
Conversation
Matt's point — miniflare D1 is a local SQLite file, strongly consistent, so a read-after-write 'lag' isn't possible; a missed read would be a real code bug. Investigated both: - i18n translations: reproduced against live D1 — the backend is CORRECT (the translations API returns both siblings, translation_group is right). The failure is front-end render timing on the slower workerd dev runtime, not a D1 bug. Comment corrected. - invite-flow: couldn't reach the user-creation path without the passkey ceremony; re-characterized as a possible real SQLite-vs-D1-dialect bug needing a browser-driven repro, no longer hand-waved as a dev consistency artifact.
|
github-actions
Bot
added
review/needs-review
There was a problem hiding this comment.
Pull request overview
This PR updates the Cloudflare/workerd-specific skip annotations in the e2e suite to reflect the investigated root causes more accurately (comment + skip-reason string adjustments only), keeping the Cloudflare lane green while documenting the current understanding of the failures.
Changes:
- Rewrites the
FIXME(cloudflare)rationale and skip message for the invite-flow “invited user appears in the users list” spec. - Rewrites the
FIXME(cloudflare)rationale and skip message for the i18n “shows Edit link for existing translations” spec.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| e2e/tests/invite-flow.spec.ts | Updates Cloudflare-skip commentary and skip reason string for the passkey invite user-list spec. |
| e2e/tests/i18n.spec.ts | Updates Cloudflare-skip commentary and skip reason string for the translations sidebar “Edit” link timing spec. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // issue specific to the slower workerd dev runtime — the sibling | ||
| // `clickEditTranslation` test (208) passes because `.click()` waits | ||
| // longer than this `isVisible` check. Skipped so the CF lane stays green. |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
docs | 33476b8 | Jun 04 2026, 08:30 AM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-playground | 33476b8 | Jun 04 2026, 08:31 AM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-cache | 33476b8 | Jun 04 2026, 08:31 AM |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR correctly fixes misleading skip-reason characterizations from #1322. The reasoning is sound: miniflare D1 is a local SQLite file, so read-after-write "consistency" artifacts are not possible — if a read misses a committed write it indicates a real code bug or a front-end timing issue, not a dev-environment consistency lag. The comment corrections and updated skip strings are appropriate.
I checked the two changed files, traced the referenced helper methods (hasEditTranslationLink, clickEditTranslation) in e2e/fixtures/admin.ts, and verified there are no behavior changes, no AGENTS.md convention violations, and no changeset requirement (test-only). The implementation is clean.
One minor suggestion: a stale line-number reference in the i18n.spec.ts comment that should be corrected before merge.
| // D1 returns both siblings correctly (EN's translation_group is | ||
| // self-referential, FR's points to EN). It's a front-end render-timing | ||
| // issue specific to the slower workerd dev runtime — the sibling | ||
| // `clickEditTranslation` test (208) passes because `.click()` waits |
There was a problem hiding this comment.
[suggestion] The comment references (208) as the line of the sibling clickEditTranslation test, but in this file clickEditTranslation is called at line 248 (and 298); the test definition itself is at line 221. In the base branch it was at line 246. The 208 reference appears stale or copied from an intermediate revision and will mislead anyone following the breadcrumb in the future.
| // `clickEditTranslation` test (208) passes because `.click()` waits | |
| // `clickEditTranslation` test (line 248) passes because `.click()` waits |
github-actions
Bot
added
review/awaiting-author
ascorbic
changed the title
github-actions
Bot
added
review/needs-rereview
ascorbic
force-pushed
the
fix/cf-skip-comments
branch
from
9cf5f0d to
33476b8
Compare
ascorbic
changed the title
github-actions
Bot
added
review/awaiting-author
What does this PR do?
Follow-up to #1322. Corrects the
FIXME(cloudflare)comments on the two CF-skipped tests after investigating them — the merged PR pinned both on "suspected D1 Sessions read-after-write," which is wrong (miniflare D1 is a strongly-consistent local SQLite file, so a read-after-write lag is impossible).Both tests stay skipped, now with accurate characterizations:
i18n › shows Edit link for existing translations— backend verified correct on D1 (the translations API returns both siblings). The flake is a dev-runtime artifact: the workerd dev runner's Vite optimizer re-bundles mid-test and stalls the translations-sidebar fetch, so the "Edit" link occasionally renders past the helper's timeout. I explored a fix — a browser-based admin-chunk warm-up inglobal-setup— and it made the test reliable locally (~2.4s), but on CI's slower 2-core runner the render still exceeded a reasonable timeout, and the resource-heavy warm-up destabilized other shards. So it's reverted; the test stays skipped with the accurate comment. Not a product bug (production has no Vite optimizer).invite-flow › invited user appears in the users list— passkey-gated write path I couldn't reach to verify. Re-characterized: if the read genuinely misses the write on strongly-consistent local D1, it's a real code bug (likely a SQLite-vs-D1-dialect difference), not a dev artifact. Flagged for maintainers; needs a browser-driven repro.No behavior change — comments + skip-reason strings only.
Closes #
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain.AI-generated code disclosure
Screenshots / test output
N/A — comment + skip-reason corrections only.