Copilot commented on code in PR #39980:
URL: https://github.com/apache/superset/pull/39980#discussion_r3270355646
##########
superset-frontend/playwright/tests/dashboard/dashboard-list.spec.ts:
##########
@@ -81,20 +84,18 @@ test('should delete a dashboard with confirmation', async ({
// Type "DELETE" to confirm
await deleteModal.fillConfirmationInput('DELETE');
- // Click the Delete button
+ // Click the Delete button (waits for it to become enabled)
await deleteModal.clickDelete();
// Modal should close
await deleteModal.waitForHidden();
- // Verify success toast appears
+ // Verify success toast appears.
const toast = new Toast(page);
await expect(toast.getSuccess()).toBeVisible();
Review Comment:
PR description says toast assertions were switched to a Locator.waitFor({
state: 'visible' }) style to better handle fast auto-dismiss, but this test
still uses expect(...).toBeVisible() for the success toast. Please align the
implementation (or update the PR description) so the de-flaking strategy is
consistent and documented accurately.
##########
superset-frontend/playwright/tests/dataset/dataset-list.spec.ts:
##########
@@ -256,6 +266,11 @@ test('should export multiple datasets via bulk select
action', async ({
datasetListPage,
testAssets,
}) => {
+ // Chains create×2 → refresh → bulk select → export. Matches the
+ // sibling bulk-delete test's budget so the export response wait below
+ // can exceed the 30s default without hitting the test timeout.
Review Comment:
The bulk-export test comment says it “matches the sibling bulk-delete test's
budget”, but the bulk-delete test in this file does not set an extended timeout
(it still relies on the 30s default). Either update the comment, or set the
bulk-delete test timeout to TIMEOUT.SLOW_TEST for consistency with
chart/dashboard list specs.
##########
superset-frontend/playwright/tests/chart/chart-list.spec.ts:
##########
@@ -81,12 +85,12 @@ test('should delete a chart with confirmation', async ({
// Modal should close
await deleteModal.waitForHidden();
- // Verify success toast appears
+ // Verify success toast appears.
const toast = new Toast(page);
await expect(toast.getSuccess()).toBeVisible();
Review Comment:
PR description says toast assertions were switched to a Locator.waitFor({
state: 'visible' }) style to better handle fast auto-dismiss, but this test
still uses expect(...).toBeVisible() for the success toast. Please align the
implementation (or update the PR description) so the de-flaking strategy is
consistent and documented accurately.
##########
superset-frontend/playwright/tests/dataset/dataset-list.spec.ts:
##########
@@ -126,14 +129,13 @@ test('should delete a dataset with confirmation', async ({
// Modal should close
await deleteModal.waitForHidden();
- // Verify success toast appears with correct message
+ // Verify success toast appears with correct message.
const toast = new Toast(page);
- const successToast = toast.getSuccess();
- await expect(successToast).toBeVisible();
+ await expect(toast.getSuccess()).toBeVisible();
await expect(toast.getMessage()).toContainText('Deleted');
Review Comment:
PR description says toast assertions were switched to a Locator.waitFor({
state: 'visible' }) style to better handle fast auto-dismiss, but this test
still uses expect(...).toBeVisible() for the success toast. Please align the
implementation (or update the PR description) so the de-flaking strategy is
consistent and documented accurately.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
