[ci] Add additional Dreamverse UI tests - #1417
Conversation
mergify
Bot
added
type: ci
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🔴 PR merge requirementsWaiting for
This rule is failing.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a silent stereo AAC audio track to the mock server's video generation and introduces comprehensive end-to-end tests for streaming, playback, downloading, and project persistence. It also expands Playwright test coverage across multiple desktop and mobile browsers. The review feedback focuses on improving test robustness by replacing hardcoded 'just now' time assertions with regular expressions to prevent CI flakiness, and using Object.defineProperty to mock navigator.share to avoid potential TypeError issues in modern browsers.
| (navigator as unknown as { share: (d: { files?: File[] }) => Promise<void> }).share = async (data) => { | ||
| const files = Array.isArray(data?.files) ? data.files : []; | ||
| (window as unknown as { __sharedFiles: unknown }).__sharedFiles = files.map((f) => ({ | ||
| name: f.name, | ||
| type: f.type, | ||
| size: f.size, | ||
| })); | ||
| }; |
There was a problem hiding this comment.
Directly assigning to navigator.share can throw a TypeError in modern browsers (like Safari/WebKit or mobile Chrome) because navigator properties are typically read-only on the prototype.\n\nTo ensure the mock works reliably across all target browsers (including WebKit and mobile-safari), use Object.defineProperty to define the property as writable and configurable.
Object.defineProperty(navigator, 'share', {
value: async (data?: { files?: File[] }) => {
const files = Array.isArray(data?.files) ? data.files : [];
(window as unknown as { __sharedFiles: unknown }).__sharedFiles = files.map((f) => ({
name: f.name,
type: f.type,
size: f.size,
}));
},
configurable: true,
writable: true,
});| await page.getByRole('button', { name: 'Toggle sidebar' }).click(); | ||
| await expect(sidebar).toBeInViewport(); | ||
| await expect(sidebar.getByText('Previous', { exact: true })).toBeVisible({ timeout: 30_000 }); | ||
| await expect(sidebar.getByText('just now').first()).toBeVisible(); |
There was a problem hiding this comment.
Using the hardcoded string 'just now' can lead to flaky tests in CI environments if there is any delay between the generation completing and the sidebar assertion.\n\nUsing a regular expression that matches both 'just now' and relative minute durations (similar to the assertion on line 298) will make the test much more robust.
| await expect(sidebar.getByText('just now').first()).toBeVisible(); | |
| await expect(sidebar.getByText(/^(just now|\d+m ago)$/).first()).toBeVisible(); |
| }); | ||
|
|
||
| await test.step('clicking the prior session enters viewing mode', async () => { | ||
| const priorRow = sidebar.locator('div[role="button"]').filter({ hasText: 'just now' }).first(); |
There was a problem hiding this comment.
Using the hardcoded string 'just now' to filter the prior session row can cause the test to fail if the relative time updates to '1m ago' due to CI latency.\n\nUpdating this to use a regular expression matching both 'just now' and relative minute durations will prevent test flakiness.
| const priorRow = sidebar.locator('div[role="button"]').filter({ hasText: 'just now' }).first(); | |
| const priorRow = sidebar.locator('div[role="button"]').filter({ hasText: /^(just now|\d+m ago)$/ }).first(); |
| await page.getByRole('button', { name: 'Toggle sidebar' }).click(); | ||
| await expect(sidebar).toBeInViewport(); | ||
| await expect(sidebar.getByText('Previous', { exact: true })).toBeVisible({ timeout: 30_000 }); | ||
| await expect(sidebar.getByText('just now').first()).toBeVisible(); |
There was a problem hiding this comment.
Using the hardcoded string 'just now' here can cause flakiness if the page reload or preceding steps take longer than a minute in CI.\n\nUsing a regular expression matching both 'just now' and relative minute durations (as done on line 298) ensures consistency and robustness.
| await expect(sidebar.getByText('just now').first()).toBeVisible(); | |
| await expect(sidebar.getByText(/^(just now|\d+m ago)$/).first()).toBeVisible(); |
Pre-commit checks failedHi @kevin314, the pre-commit checks have failed. To fix them locally: # Install pre-commit if you haven't already
uv pip install pre-commit
pre-commit install
# Run all checks and auto-fix what's possible
pre-commit run --all-filesCommon fixes:
After fixing, commit and push the changes. The checks will re-run automatically. For future commits, |
Address review S1s on PR #1417: - Use Object.defineProperty on Navigator.prototype for share/canShare stubs (direct assignment silently no-ops on Chromium/WebKit where navigator.share is a non-writable accessor). - Replace exact-text 'just now' assertions with the regex pattern\n already used at the third site; avoids minute-boundary flake on\n slow CI runs.
|
Both S1s from the prior review have been pushed.
The S2 (msedge orphan in browser matrix) is unchanged — separate cleanup decision for the maintainer. — Gob (@SolitaryThinker's AI reviewer, posting on his behalf) |
Pre-commit checks failedHi @kevin314, the pre-commit checks have failed. To fix them locally: # Install pre-commit if you haven't already
uv pip install pre-commit
pre-commit install
# Run all checks and auto-fix what's possible
pre-commit run --all-filesCommon fixes:
After fixing, commit and push the changes. The checks will re-run automatically. For future commits, |
|
Hi @kevin314 — automated re-review from Gob, one of @SolitaryThinker's AI reviewers. Findings aren't all human-verified; ping @SolitaryThinker if anything looks off. TL;DRBoth prior S1s are closed cleanly at Verdict: approve-with-followup (was: ship-with-fixes)
Prior findings status
New findings(none) — Gob (@SolitaryThinker's AI reviewer). Full review (including the new-work audit and S3 items) is archived locally. |
1 similar comment
|
Hi @kevin314 — automated re-review from Gob, one of @SolitaryThinker's AI reviewers. Findings aren't all human-verified; ping @SolitaryThinker if anything looks off. TL;DRBoth prior S1s are closed cleanly at Verdict: approve-with-followup (was: ship-with-fixes)
Prior findings status
New findings(none) — Gob (@SolitaryThinker's AI reviewer). Full review (including the new-work audit and S3 items) is archived locally. |
|
/merge |
Pre-commit checks failedHi @kevin314, the pre-commit checks have failed. To fix them locally: # Install pre-commit if you haven't already
uv pip install pre-commit
pre-commit install
# Run all checks and auto-fix what's possible
pre-commit run --all-filesCommon fixes:
After fixing, commit and push the changes. The checks will re-run automatically. For future commits, |
Pre-commit yapf (--all-files --hook-stage manual) flagged line-wrap drift in two files outside PR #1417's diff. Bundling the minimal reflow here so the PR's pre-commit gate goes green. No logic change.
2 participants
Purpose
Adds additional playwright tests for the Dreamverse UI
Changes
New coverage for chromium as well as Safari, Firefox, Edge (on desktop and mobile):
Test Plan
cd apps/dreamverse/web npm run e2eTest Results
Test output
Checklist
pre-commit run --all-filesand fixed all issuesFor model/pipeline changes, also check: