aminghadersohi commented on code in PR #44856:
URL: https://github.com/apache/superset/pull/44856#discussion_r4161231104
##########
superset-frontend/playwright/tests/explore/explore-save.spec.ts:
##########
@@ -116,7 +116,11 @@ testWithAssets(
pathMatch: true,
});
await saveModal2.clickSave();
- expect((await updated).ok()).toBe(true);
+ const updatedResponse = await updated;
+ expect(updatedResponse.ok()).toBe(true);
+ expect(updatedResponse.request().postDataJSON().dashboards).toContain(
+ dashboardId,
+ );
Review Comment:
Agreed, `toEqual([dashboardId])` is the stricter check. Not pushing it on
this approved, green head, since a push restarts the full CI run.
##########
superset-frontend/playwright/tests/explore/cross-referenced-dashboards.spec.ts:
##########
@@ -50,7 +51,11 @@ async function overwriteToDashboard(
pathMatch: true,
});
await saveModal.clickSave();
- expect((await updated).ok()).toBe(true);
+ const updatedResponse = await updated;
+ expect(updatedResponse.ok()).toBe(true);
+ expect(updatedResponse.request().postDataJSON().dashboards).toContain(
+ dashboardId,
+ );
// A duplicate here means the save created a new dashboard instead of
// picking the existing one; it would also escape testAssets cleanup.
Review Comment:
Agreed, the count is a backstop here and the dashboards check is what
catches the prefix-labelled pick. Comment-only, so not worth restarting CI on
an approved, green head.
##########
superset-frontend/playwright/tests/mobile/mobile-dashboard.spec.ts:
##########
@@ -24,50 +24,39 @@ import { test, expect, devices, Page } from
'@playwright/test';
// environment (FEATURE_FLAGS = {"MOBILE_CONSUMPTION_MODE": True}).
import { TIMEOUT } from '../../utils/constants';
import { URL } from '../../utils/urls';
+import { gotoWithRetry } from '../../helpers/navigation';
/**
* Mobile dashboard viewing tests verify that dashboards can be viewed
* and interacted with on mobile devices.
*
- * These tests assume the World Bank's Health sample dashboard exists.
+ * These tests assume the World Bank's Data sample dashboard exists.
*/
// Use iPhone 12 viewport for mobile tests
const mobileViewport = devices['iPhone 12'];
-/**
- * Navigates to the dashboard list, clicks the first available dashboard
- * card, and waits for navigation into that dashboard. Skips the current
- * test when no dashboards are available to open.
- */
-async function openFirstDashboard(page: Page): Promise<void> {
- await page.goto(URL.DASHBOARD_LIST);
+/** Opens the required sample dashboard and asserts navigation succeeds. */
+async function openExampleDashboard(page: Page): Promise<void> {
+ const response = await gotoWithRetry(page, 'dashboard/world_health/');
+ expect(
+ response?.status(),
+ 'world_health missing; run superset load_examples',
+ ).toBe(200);
Review Comment:
Agreed, a redirect to /login/ would pass the 200 check; the test would still
fail at the next dashboard assertion, just with a less specific message. Not
pushing it on this approved, green head, since that restarts CI.
##########
superset-frontend/playwright/tests/mobile/mobile-dashboard.spec.ts:
##########
@@ -24,50 +24,39 @@ import { test, expect, devices, Page } from
'@playwright/test';
// environment (FEATURE_FLAGS = {"MOBILE_CONSUMPTION_MODE": True}).
import { TIMEOUT } from '../../utils/constants';
import { URL } from '../../utils/urls';
+import { gotoWithRetry } from '../../helpers/navigation';
/**
* Mobile dashboard viewing tests verify that dashboards can be viewed
* and interacted with on mobile devices.
*
- * These tests assume the World Bank's Health sample dashboard exists.
+ * These tests assume the World Bank's Data sample dashboard exists.
*/
// Use iPhone 12 viewport for mobile tests
const mobileViewport = devices['iPhone 12'];
-/**
- * Navigates to the dashboard list, clicks the first available dashboard
- * card, and waits for navigation into that dashboard. Skips the current
- * test when no dashboards are available to open.
- */
-async function openFirstDashboard(page: Page): Promise<void> {
- await page.goto(URL.DASHBOARD_LIST);
+/** Opens the required sample dashboard and asserts navigation succeeds. */
+async function openExampleDashboard(page: Page): Promise<void> {
+ const response = await gotoWithRetry(page, 'dashboard/world_health/');
+ expect(
+ response?.status(),
+ 'world_health missing; run superset load_examples',
+ ).toBe(200);
await page.waitForLoadState('networkidle');
-
- const cards = page.locator('[data-test="styled-card"]');
- const cardCount = await cards.count();
-
- test.skip(cardCount === 0, 'No dashboards available to open on mobile');
-
- await cards.first().click();
-
- await page.waitForURL(url => /\/dashboard\/(?!list)/.test(url.pathname), {
- timeout: TIMEOUT.PAGE_LOAD,
- });
}
/**
- * Navigates to the World Bank's Health dashboard and returns a locator
+ * Navigates to the World Bank's Data dashboard and returns a locator
* for its mobile filter button. Skips the current test when the fixture
* has no native filters configured.
*/
async function getMobileFilterButton(page: Page) {
- // Navigate directly to the World Bank's Health dashboard, which this
+ // Navigate directly to the World Bank's Data dashboard, which this
// spec's fixtures require, rather than an arbitrary first card from
// the list. Whether it has native filters configured depends on the
// fixture, so callers skip themselves when none are present.
Review Comment:
Agreed on the wording. Comment-only, so not worth restarting CI on an
approved, green head.
--
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]
