Skip to content

test: prevent indexeddb race conditions #36347

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 18, 2025

Conversation

Skn0tt
Copy link
Member

@Skn0tt Skn0tt commented Jun 18, 2025

Test fails on Firefox CI sometimes - my hunch is that it's a race condition where either:

  • we submit the task before the JS is wired up OR
  • we capture storage state before the transaction committed.

Luckily, the test site already has a log available we can use to wait for both. Let's hope this helps.

@Skn0tt Skn0tt requested a review from Copilot June 18, 2025 09:17
@Skn0tt Skn0tt self-assigned this Jun 18, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

The PR adds aria snapshot checks to stabilize the IndexedDB test by waiting for log messages before capturing storage state.

  • Added initial check for “Database initialised.” notification.
  • Added post-click check for “Transaction completed: database modification finished.” notification.
Comments suppressed due to low confidence (1)

tests/library/browsercontext-storage-state.spec.ts:372

  • [nitpick] Using a full aria snapshot match can be brittle. Consider targeting the specific notification text with locator.waitFor() or expect(locator).toHaveText() for a more focused and maintainable check.
  await expect(page.locator('#notifications')).toMatchAriaSnapshot(`

await page.getByLabel('Task title').fill('Pet the cat');
await page.getByLabel('Hours').fill('1');
await page.getByLabel('Mins').fill('1');
await page.getByText('Add Task').click();
await expect(page.locator('#notifications')).toMatchAriaSnapshot(`
- list:
- listitem: "Transaction completed: database modification finished."
Copy link
Preview

Copilot AI Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The quotes around the notification text in the aria snapshot will be matched literally. Remove the surrounding quotes so the matcher reflects the actual list item content.

Suggested change
- listitem: "Transaction completed: database modification finished."
- listitem: Transaction completed: database modification finished.

Copilot uses AI. Check for mistakes.

@Skn0tt Skn0tt merged commit 2973b0b into microsoft:main Jun 18, 2025
28 of 29 checks passed
Copy link
Contributor

Test results for "tests 1"

1 failed
❌ [webkit-library] › library/browsercontext-pages.spec.ts:82:3 › should click the button with offset with page scale @webkit-ubuntu-22.04-node18

6 flaky ⚠️ [firefox-library] › library/inspector/cli-codegen-1.spec.ts:986:7 › cli codegen › should not throw csp directive violation errors @firefox-ubuntu-22.04-node18
⚠️ [firefox-page] › page/page-wait-for-function.spec.ts:104:3 › should work with strict CSP policy @firefox-ubuntu-22.04-node18
⚠️ [webkit-library] › library/browsercontext-pages.spec.ts:105:3 › should return bounding box with page scale @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › page/page-screenshot.spec.ts:345:5 › page screenshot › should work while navigating @webkit-ubuntu-22.04-node18
⚠️ [playwright-test] › ui-mode-test-watch.spec.ts:145:5 › should watch all @windows-latest-node18-1
⚠️ [playwright-test] › ui-mode-trace.spec.ts:154:5 › should show snapshots for steps @windows-latest-node18-1

39414 passed, 822 skipped
✔️✔️✔️

Merge workflow run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants