Conversation
5033e40 to
f24af0c
Compare
5ad5cf0 to
a97ca84
Compare
3e950e4 to
9e36820
Compare
|
After the most recent Stormbox updates this new test no longer works. Moving back into WIP. |
9e36820 to
1c262a7
Compare
1c262a7 to
45c8cad
Compare
Ok I've updated the new folder management test and the UI smoke test to work with the latest Webmail changes, and this patch is now ready for review. |
| }); | ||
|
|
||
| test('add, rename, move, search, and delete folders', async ({ page }, testInfo) => { | ||
| let foldersToCleanUp: Array<string> = []; |
There was a problem hiding this comment.
There is no cleanup path if anything fails. Every folder this test creates is tracked only in this local, and it is only deleted in the last step, so any failure before then leaves E2E-<ts>-* folders on the shared prod account. On CI (retries: 1) the retry runs in a fresh worker, so fNamePrefix (line 13) gets a new timestamp and the first attempt's folders are orphaned permanently. This runs nightly against prod.
Please add a test.afterEach (or try/finally) that opens Manage Folders, searches for the prefix, selects everything it finds and bulk-deletes, and ideally a pre-test sweep for stale E2E- folders from earlier runs, along the lines of sweepOrphanTestMessages in tests/e2e/helpers/.
| }); | ||
| await this.assertAccountMenuItemsVisible(); | ||
| await expect(this.selectAllMessagesCheckbox).toBeVisible(); | ||
| await expect(this.messageCount).toBeVisible(); |
There was a problem hiding this comment.
This doesn't achieve the "works on an empty inbox" goal. SelectableListHeader.vue:104-109 renders no text in the count <span> when totalCount === 0, and the span has no padding, so it is zero-width and toBeVisible() (which requires a non-empty bounding box) fails.
On current main (already on stage) the count is also hidden whenever the list is narrower than 520 px (HEADER_COUNT_MIN_WIDTH in useMessageListHeader.ts), which includes the Pixel 7 viewport, so the mobile smoke test will start failing on prod as soon as the multi-column release ships.
Assert on the header (.msg-list__header) instead, or use toBeAttached() here.
| const starButton = page.getByRole('button', { | ||
| name: `Star folder ${fName}`, | ||
| }); | ||
|
|
||
| // the folder is new so shouldn't be starred yet | ||
| expect(await starButton.getAttribute('aria-pressed')).toBe('false'); | ||
|
|
||
| // star it and verify | ||
| console.log(`starring folder: ${fName}`); | ||
| await starButton.click(); | ||
| expect(await starButton.getAttribute('aria-pressed')).toBe('true'); |
There was a problem hiding this comment.
Two problems here:
expect(await starButton.getAttribute('aria-pressed')).toBe('true')right afterclick()is a non-retrying read.setFolderStarredgoes through the DB worker asynchronously, so this can readfalseand flake. Useawait expect(starButton).toHaveAttribute('aria-pressed', 'true')(and the same for the'false'check).- The locator is
name: 'Star folder X', but after starring the label becomesUnstar folder X(FolderManagerDialog.vue:1150-1152). It keeps matching only because non-exact role matching is a case-insensitive substring match. Use[data-folder-star="X"], which the app exposes for this.
| const firstFolder = page | ||
| .getByRole('heading', { name: 'Folders', exact: true }) | ||
| .locator('xpath=..') | ||
| .locator('xpath=following-sibling::div[contains(@class, "folder-node")][1]'); | ||
|
|
||
| await expect(firstFolder.locator('.folder-node__name')).toHaveText(fName); |
There was a problem hiding this comment.
starredUserFolders is sorted by name (FolderTree.vue:100-105), so "first .folder-node after the Folders heading" equals fName only if no other starred folder sorts before E2E-…. A starred folder left behind by a failed run, or one a human starred on this account with a name below E, makes this fail on every run until someone cleans up manually.
Assert membership in the favorites group instead, e.g.
await expect(
page.locator('.folder-node[data-tour="folder-favorites"]').filter({ hasText: fName }),
).toBeVisible();| this.manageFoldersText = this.manageFoldersDialog.getByText('Drag a folder to move it, or select several to delete them'); | ||
| this.manageFoldersSearchInput = this.manageFoldersDialog.locator('.folder-subs__search-input'); | ||
| this.manageFoldersCloseBtn = this.manageFoldersDialog.getByRole('button', { name: 'Close manage folders' }); | ||
| this.manageFoldersExpandBtn = this.manageFoldersDialog.getByRole('button', { name: 'Expand default folders' }); | ||
| this.manageFoldersAddTopLevelBtn = this.manageFoldersDialog.getByRole('button', { name: 'New folder', exact: true }); | ||
| this.manageFoldersNewFolderDialog = page.getByRole('dialog', { name: 'New folder' }); | ||
| this.manageFoldersNewFolderNameInput = this.manageFoldersNewFolderDialog.getByRole('textbox', { name: 'Name' }); | ||
| this.manageFoldersNewFolderParentSelect = this.manageFoldersNewFolderDialog.locator('select[data-folder-create-parent]'); | ||
| this.manageFoldersNewFolderCreateBtn = this.manageFoldersNewFolderDialog.getByRole('button', { name: 'Create' }); | ||
| this.manageFoldersNewFolderCancelBtn = this.manageFoldersNewFolderDialog.getByRole('button', { name: 'Cancel' }); | ||
| this.manageFoldersNewFolderNameExistsText = this.manageFoldersNewFolderDialog.getByText('A folder with that name already exists here.', { exact: true }); | ||
| this.manageFoldersExpandInboxBtn = this.manageFoldersDialog.getByRole('button', { name: 'Expand inbox' }); |
There was a problem hiding this comment.
Prefer the data-folder-* hooks the app ships for tests over visible copy. FolderManagerDialog.vue / FolderCreateDialog.vue expose data-folder-new, data-folder-create-name, data-folder-create-submit, data-folder-toggle="<name>", data-folder-edit="<name>", data-folder-rename-input, data-folder-save, data-folder-select="<name>", data-folder-star="<name>", data-account-toggle, and tests/e2e/folder-crud.spec.js already drives the dialog with them.
Concrete examples in this block: 'Expand inbox' only matches the real label Expand Inbox because non-exact role matching is case-insensitive ([data-folder-toggle="Inbox"] is unambiguous); the hint sentence and 'Create' are copy that can change without anyone thinking of this suite. Copy changes already broke this test once (your Sep 10 comment); hooks would have survived that.
| await expect( | ||
| page.locator('.folder-subs__name').getByText(fName, { exact: true }) | ||
| ).toBeVisible() |
There was a problem hiding this comment.
The dialog virtualizes its rows (renderedItems / TanStack virtualizer in FolderManagerDialog.vue), so rows below the fold are not in the DOM at all and toBeVisible() fails for them. This works today because the list is short, but it will break as the account accumulates folders (see the cleanup comment). Filtering with the search box before asserting, as the delete step already does, is robust to that; the same applies to the other .folder-subs__name visibility checks in this file.
|
|
||
| // verify default folders (we already expanded the folders list in openManageFoldersDialog) | ||
| for (const folderName of FOLDER_NAMES_TO_EXERCISE) { | ||
| await expect(this.page.locator('.folder-subs__name', { hasText: folderName })).toBeVisible(); |
There was a problem hiding this comment.
page.locator('.folder-subs__name', { hasText: folderName }) matches every row containing that text anywhere on the page, including a shared account's Inbox / Sent Items if the test account has shared folders, and toBeVisible() throws a strict-mode violation on multiple matches. Scope it to this.manageFoldersDialog and the own-account section (or at least use getByText(folderName, { exact: true }) and .first()).
| private async isInboxEmptyTextVisible(timeout: number) { | ||
| try { | ||
| await expect(this.inboxEmptyText).toBeVisible({ timeout }); | ||
| return true; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Dead code: isInboxEmptyTextVisible is never called, and inboxEmptyText (declared at line 53, assigned at line 129) is only referenced here. Please drop all three.
| if (duplicate) { | ||
| await expect(this.manageFoldersNewFolderNameExistsText).toBeVisible(); | ||
| await this.manageFoldersNewFolderCancelBtn.click({ force: projectName.toLowerCase().includes('android')}); | ||
| } else { |
There was a problem hiding this comment.
Dead path: every addFolder call in folder-management.spec.ts passes duplicate = false, so this branch and manageFoldersNewFolderNameExistsText are never exercised. Either add a step that creates a duplicate name (the app has a specific error for it, worth one assertion) or remove the flag and the locator.
What changed?
Add E2E tests for managing folders in WebMail, and update the UI smoke test also.
Why?
Expand the current E2E test suite that runs in BrowserStack on desktop and mobile browsers.
Limitations and Notes
More specifically:
Manage Foldersdialog controls (basic check only)Manage FoldersE2E test to actually add/modify/delete folders via theManage Foldersdialog1.67.0I tested this out with these E2E tests in BrowserStack and it worked fineI worked with Codex AI to make this patch. I reviewed all code changes and tested thoroughly.
Applicable Issues
Fixes #70.
QA Log
I ran the updated E2E tests (UI smoke test and the new folder management test):
All tests pass.