sadpandajoe commented on code in PR #36196: URL: https://github.com/apache/superset/pull/36196#discussion_r2583887411
########## superset-frontend/playwright/utils/constants.ts: ########## @@ -0,0 +1,47 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/** + * Timeout constants for Playwright tests. + * Only define timeouts that differ from Playwright defaults or are semantically important. + * + * Default Playwright timeouts (from playwright.config.ts): + * - Test timeout: 30000ms (30s) + * - Expect timeout: 8000ms (8s) + * + * Use these constants instead of magic numbers for better maintainability. + */ + +export const TIMEOUT = { + /** + * Page navigation and load timeouts + */ + PAGE_LOAD: 10000, // 10s for page transitions (login → welcome, dataset → explore) + + /** + * Form and UI element load timeouts + */ + FORM_LOAD: 5000, // 5s for forms to become visible (login form, modals) + + /** + * API polling intervals + */ + API_POLL_INTERVAL: 100, // 100ms between API polling attempts Review Comment: Good catch - the constant exists but isn't used. The 500ms interval is inlined in AuthPage for clarity. Will consider using the constant in future refactoring. ########## superset-frontend/playwright/pages/AuthPage.ts: ########## @@ -83,6 +84,67 @@ export class AuthPage { await loginButton.click(); } + /** + * Wait for successful login by verifying the login response and session cookie. + * Call this after loginWithCredentials to ensure authentication completed. + * + * This does NOT assume a specific landing page (which is configurable). + * Instead it: + * 1. Checks if session cookie already exists (guards against race condition) + * 2. Waits for POST /login/ response with redirect status + * 3. Polls for session cookie to appear + * + * @param options - Optional wait options + */ + async waitForLoginSuccess(options?: { timeout?: number }): Promise<void> { + const timeout = options?.timeout ?? TIMEOUT.PAGE_LOAD; + const startTime = Date.now(); + + // 1. Guard: Check if session cookie already exists (race condition protection) + const existingCookie = await this.getSessionCookie(); + if (existingCookie?.value) { + // Already authenticated - login completed before we started waiting + return; + } + + // 2. Wait for POST /login/ response (bounded by caller's timeout) + const loginResponse = await this.page.waitForResponse( + response => + response.url().includes('/login/') && + response.request().method() === 'POST', + { timeout }, + ); + + // 3. Verify it's a redirect (3xx status code indicates successful login) + const status = loginResponse.status(); + if (status < 300 || status >= 400) { + throw new Error(`Login failed: expected redirect (3xx), got ${status}`); + } + + // 4. Poll for session cookie to appear (HttpOnly cookie, not accessible via document.cookie) + // Use page.context().cookies() since session cookie is HttpOnly + const pollInterval = 500; // 500ms instead of 100ms for less chattiness + while (true) { + const remaining = timeout - (Date.now() - startTime); + if (remaining <= 0) { + break; // Timeout exceeded + } + + const sessionCookie = await this.getSessionCookie(); + if (sessionCookie && sessionCookie.value) { + // Success - session cookie has landed + return; + } + + await this.page.waitForTimeout(Math.min(pollInterval, remaining)); Review Comment: The polling is necessary because Superset's session cookie is HttpOnly - it can't be detected via `document.cookie` in a `waitForFunction`. We use `page.context().cookies()` which requires polling from the test context. ########## superset-frontend/playwright/components/core/Modal.ts: ########## @@ -0,0 +1,118 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { Locator, Page } from '@playwright/test'; + +/** + * Base Modal component for Ant Design modals. + * Provides minimal primitives - extend this for specific modal types. + * Add methods to this class only when multiple modal types need them (YAGNI). + * + * @example + * class DeleteConfirmationModal extends Modal { + * async clickDelete(): Promise<void> { + * await this.footer.locator('button', { hasText: 'Delete' }).click(); + * } + * } + */ +export class Modal { + protected readonly page: Page; + protected readonly modalSelector: string; + + // Ant Design modal structure selectors (shared by all modal types) + protected static readonly BASE_SELECTORS = { + FOOTER: '.ant-modal-footer', + BODY: '.ant-modal-body', + }; + + constructor(page: Page, modalSelector = '[role="dialog"]') { + this.page = page; + this.modalSelector = modalSelector; + } + + /** + * Gets the modal element locator + */ + get element(): Locator { + return this.page.locator(this.modalSelector); + } + + /** + * Gets the modal footer locator (contains action buttons) + */ + get footer(): Locator { + return this.element.locator(Modal.BASE_SELECTORS.FOOTER); + } + + /** + * Gets the modal body locator (contains content) + */ + get body(): Locator { + return this.element.locator(Modal.BASE_SELECTORS.BODY); + } + + /** + * Gets a footer button by text content (private helper) + * @param buttonText - The text content of the button + */ + private getFooterButton(buttonText: string): Locator { + return this.footer.getByRole('button', { name: buttonText, exact: true }); + } + + /** + * Clicks a footer button by text content + * @param buttonText - The text content of the button to click + * @param options - Optional click options + */ + protected async clickFooterButton( + buttonText: string, + options?: { timeout?: number; force?: boolean; delay?: number }, + ): Promise<void> { + await this.getFooterButton(buttonText).click(options); + } + + /** + * Waits for the modal to become visible + * @param options - Optional wait options + */ + async waitForVisible(options?: { timeout?: number }): Promise<void> { + await this.element.waitFor({ state: 'visible', ...options }); + } + + /** + * Waits for the modal to be fully ready for interaction. + * This includes waiting for the modal dialog to be visible AND for React to finish + * rendering the modal content. Use this before interacting with modal elements + * to avoid race conditions with React state updates. + * + * @param options - Optional wait options + */ + async waitForReady(options?: { timeout?: number }): Promise<void> { + await this.waitForVisible(options); + await this.body.waitFor({ state: 'visible', ...options }); Review Comment: The `.ant-modal-body` visibility check is sufficient - when the body is visible, React has rendered the modal content. This is a synchronization point, not a timing hack. ########## superset-frontend/playwright/tests/experimental/dataset/dataset-list.spec.ts: ########## @@ -0,0 +1,243 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { test, expect } from '@playwright/test'; +import { DatasetListPage } from '../../../pages/DatasetListPage'; +import { ExplorePage } from '../../../pages/ExplorePage'; +import { DeleteConfirmationModal } from '../../../components/modals/DeleteConfirmationModal'; +import { DuplicateDatasetModal } from '../../../components/modals/DuplicateDatasetModal'; +import { Toast } from '../../../components/core/Toast'; +import { + apiDeleteDataset, + apiGetDataset, + getDatasetByName, + ENDPOINTS, +} from '../../../helpers/api/dataset'; + +/** + * Test data constants + * These reference example datasets loaded via --load-examples in CI + */ +const TEST_DATASETS = { + EXAMPLE_DATASET: 'members_channels_2', +} as const; Review Comment: The dataset is loaded via `--load-examples` in CI (bashlib.sh:playwright_testdata). Test will fail fast with clear error if dataset missing. Adding runtime skip logic would add complexity for little benefit. -- 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]
