rebenitez1802 commented on code in PR #44856:
URL: https://github.com/apache/superset/pull/44856#discussion_r4160814917
##########
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:
With prefix typing, a wrong "create new" pick would be titled with the
prefix, so this count stays 1. The `toContain(dashboardId)` above is what
catches it:
```suggestion
// The dashboards check above catches a pick of the prefix-labelled
// "create new" option; this exact-title count is a backstop for any
// other path creating a same-title duplicate that would escape cleanup.
```
##########
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:
The chart is already on `dashboardId` from the save-as and SaveModal only
appends, so `toContain` passes even if a new dashboard is created. Asserting
the exact list catches any extra id:
```suggestion
expect(updatedResponse.request().postDataJSON().dashboards).toEqual([
dashboardId,
]);
```
##########
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:
A lost session redirects to `/login/?next=…/dashboard/world_health/`, which
returns 200. Checking the final path catches that. This uses `page.url()`
because the `URL` import in this file shadows the global constructor:
```suggestion
const response = await gotoWithRetry(page, 'dashboard/world_health/');
expect(
response?.status(),
'world_health missing; run superset load_examples',
).toBe(200);
expect(
page.url().split('?')[0],
'redirected away from world_health; is the session still valid?',
).toMatch(/\/dashboard\/world_health\/$/);
```
##########
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:
`world_health` ships with `native_filter_configuration: []`, and this helper
(not its callers) does the skip:
```suggestion
// Navigate directly to the World Bank's Data dashboard, which this
// spec's fixtures require. The shipped example has no native filters,
// so this helper skips the calling test until a filter-bearing fixture
// exists.
```
--
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]
