Copilot commented on code in PR #36196: URL: https://github.com/apache/superset/pull/36196#discussion_r2583858017
########## 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 test dataset name 'members_channels_2' is hardcoded as a constant but there's no verification that this dataset will exist in all environments. While the comment mentions it's loaded via `--load-examples` in CI, this dependency should be documented more prominently or checked at runtime. Consider adding a verification step or a clearer error message if the dataset doesn't exist: ```typescript const dataset = await getDatasetByName(page, datasetName); if (!dataset) { throw new Error( `Example dataset '${datasetName}' not found. ` + `Ensure test environment was initialized with 'superset load_examples'` ); } ``` ########## 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: The `API_POLL_INTERVAL` constant is defined here but not actually used in the codebase. The `AuthPage.waitForLoginSuccess()` method uses a hardcoded 500ms interval instead of this constant. Consider either: 1. Using this constant in `AuthPage.waitForLoginSuccess()` (line 126 of AuthPage.ts): `const pollInterval = TIMEOUT.API_POLL_INTERVAL;` 2. Or removing this unused constant if it's not needed 3. Or documenting why different poll intervals are used in different contexts ```suggestion * API polling timeout */ ``` ########## superset-frontend/playwright/tests/auth/login.spec.ts: ########## @@ -20,69 +20,74 @@ import { test, expect } from '@playwright/test'; import { AuthPage } from '../../pages/AuthPage'; import { URL } from '../../utils/urls'; +import { TIMEOUT } from '../../utils/constants'; -test.describe('Login view', () => { - let authPage: AuthPage; +// Test credentials - can be overridden via environment variables +const adminUsername = process.env.PLAYWRIGHT_ADMIN_USERNAME || 'admin'; +const adminPassword = process.env.PLAYWRIGHT_ADMIN_PASSWORD || 'general'; - test.beforeEach(async ({ page }: any) => { - authPage = new AuthPage(page); - await authPage.goto(); - await authPage.waitForLoginForm(); - }); +/** + * Auth/login tests use per-test navigation via beforeEach. + * Each test starts fresh on the login page without global authentication. + * This follows the Cypress pattern for auth testing - simple and isolated. + */ - test('should redirect to login with incorrect username and password', async ({ - page, - }: any) => { - // Setup request interception before login attempt - const loginRequestPromise = authPage.waitForLoginRequest(); +let authPage: AuthPage; - // Attempt login with incorrect credentials - await authPage.loginWithCredentials('admin', 'wrongpassword'); +test.beforeEach(async ({ page }) => { + // Navigate to login page before each test (ensures clean state) + authPage = new AuthPage(page); + await authPage.goto(); + await authPage.waitForLoginForm(); +}); - // Wait for login request and verify response - const loginResponse = await loginRequestPromise; - // Failed login returns 401 Unauthorized or 302 redirect to login - expect([401, 302]).toContain(loginResponse.status()); +test('should redirect to login with incorrect username and password', async ({ + page, +}) => { + // Setup request interception before login attempt + const loginRequestPromise = authPage.waitForLoginRequest(); - // Wait for redirect to complete before checking URL - await page.waitForURL((url: any) => url.pathname.endsWith('login/'), { - timeout: 10000, - }); + // Attempt login with incorrect credentials (both username and password invalid) + await authPage.loginWithCredentials('wronguser', 'wrongpassword'); Review Comment: The test uses both an incorrect username and incorrect password ('wronguser', 'wrongpassword'), but the test name only mentions "incorrect username and password". The comment on line 50 clarifies this, but it would be clearer if the test focused on a single authentication failure case. Consider either: 1. Testing incorrect password with valid username (more realistic scenario): `await authPage.loginWithCredentials(adminUsername, 'wrongpassword');` 2. Or renaming the test to be more specific: "should redirect to login with invalid credentials" This would make the test intent clearer and more aligned with typical user errors. ```suggestion // Attempt login with valid username and incorrect password await authPage.loginWithCredentials(adminUsername, 'wrongpassword'); ``` ########## 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 `waitForReady()` method documentation mentions waiting for "React to finish rendering the modal content" and preventing "race conditions with React state updates", but there's no explicit wait for React hydration or state updates. The implementation only waits for DOM visibility. The current implementation (`waitForVisible()` + `body.waitFor()`) doesn't actually wait for React state - it only waits for DOM elements to be visible. If React race conditions are a concern, consider: 1. Adding an explicit wait or check for React rendering completion 2. Or updating the documentation to accurately reflect what the method does (waits for modal and body to be visible) 3. Or adding a small timeout after visibility to allow for React state updates ```suggestion await this.body.waitFor({ state: 'visible', ...options }); // Allow React state updates to settle after DOM is visible await this.page.waitForTimeout(50); ``` ########## 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; + +test.describe('Dataset List', () => { + let datasetListPage: DatasetListPage; + let explorePage: ExplorePage; + let testResources: { datasetIds: number[] } = { datasetIds: [] }; + + test.beforeEach(async ({ page }) => { + datasetListPage = new DatasetListPage(page); + explorePage = new ExplorePage(page); + testResources = { datasetIds: [] }; // Reset for each test + + // Navigate to dataset list page + await datasetListPage.goto(); + await datasetListPage.waitForTableLoad(); + }); + + test.afterEach(async ({ page }) => { + // Cleanup any resources created during the test + const promises = []; + for (const datasetId of testResources.datasetIds) { + promises.push( + apiDeleteDataset(page, datasetId, { + failOnStatusCode: false, + }).catch(error => { + // Log cleanup failures to avoid silent resource leaks + console.warn( + `[Cleanup] Failed to delete dataset ${datasetId}:`, + String(error), + ); + }), + ); + } + await Promise.all(promises); + }); + + test('should navigate to Explore when dataset name is clicked', async ({ + page, + }) => { + // Use existing example dataset (hermetic - loaded in CI via --load-examples) + const datasetName = TEST_DATASETS.EXAMPLE_DATASET; + const dataset = await getDatasetByName(page, datasetName); + expect(dataset).not.toBeNull(); + + // Verify dataset is visible in list (uses page object + Playwright auto-wait) + await expect(datasetListPage.getDatasetRow(datasetName)).toBeVisible(); + + // Click on dataset name to navigate to Explore + await datasetListPage.clickDatasetName(datasetName); + + // Wait for Explore page to load (validates URL + datasource control) + await explorePage.waitForPageLoad(); + + // Verify correct dataset is loaded in datasource control + const loadedDatasetName = await explorePage.getDatasetName(); + expect(loadedDatasetName).toContain(datasetName); + + // Verify visualization switcher shows default viz type (indicates full page load) + await expect(explorePage.getVizSwitcher()).toBeVisible(); + await expect(explorePage.getVizSwitcher()).toContainText('Table'); + }); + + test('should delete a dataset with confirmation', async ({ page }) => { + // Get example dataset to duplicate + const originalName = TEST_DATASETS.EXAMPLE_DATASET; + const originalDataset = await getDatasetByName(page, originalName); + expect(originalDataset).not.toBeNull(); + + // Create throwaway copy for deletion (hermetic - uses UI duplication) + const datasetName = `test_delete_${Date.now()}`; + + // Verify original dataset is visible in list + await expect(datasetListPage.getDatasetRow(originalName)).toBeVisible(); + + // Set up response intercept to capture duplicate dataset ID + const duplicateResponsePromise = page.waitForResponse( + response => + response.url().includes(`${ENDPOINTS.DATASET}duplicate`) && + response.status() === 201, + ); + + // Click duplicate action button + await datasetListPage.clickDuplicateAction(originalName); + + // Duplicate modal should appear and be ready for interaction + const duplicateModal = new DuplicateDatasetModal(page); + await duplicateModal.waitForReady(); + + // Fill in new dataset name + await duplicateModal.fillDatasetName(datasetName); + + // Click the Duplicate button + await duplicateModal.clickDuplicate(); + + // Get the duplicate dataset ID from response and track immediately + const duplicateResponse = await duplicateResponsePromise; + const duplicateData = await duplicateResponse.json(); + const duplicateId = duplicateData.id; + + // Track duplicate for cleanup immediately (before any operations that could fail) + testResources = { datasetIds: [duplicateId] }; Review Comment: The pattern of immediately tracking resources for cleanup after creation could be extracted into a helper function to reduce duplication between tests. This appears in multiple places (lines 139 and 216) with the same structure. Consider creating a helper like: ```typescript function trackDatasetForCleanup(datasetId: number) { testResources.datasetIds.push(datasetId); } ``` This would make the intent clearer and reduce the chance of forgetting to track resources in future tests. ########## 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; + +test.describe('Dataset List', () => { + let datasetListPage: DatasetListPage; + let explorePage: ExplorePage; + let testResources: { datasetIds: number[] } = { datasetIds: [] }; + + test.beforeEach(async ({ page }) => { + datasetListPage = new DatasetListPage(page); + explorePage = new ExplorePage(page); + testResources = { datasetIds: [] }; // Reset for each test + + // Navigate to dataset list page + await datasetListPage.goto(); + await datasetListPage.waitForTableLoad(); + }); + + test.afterEach(async ({ page }) => { + // Cleanup any resources created during the test + const promises = []; + for (const datasetId of testResources.datasetIds) { + promises.push( + apiDeleteDataset(page, datasetId, { + failOnStatusCode: false, + }).catch(error => { + // Log cleanup failures to avoid silent resource leaks + console.warn( + `[Cleanup] Failed to delete dataset ${datasetId}:`, + String(error), + ); + }), + ); + } + await Promise.all(promises); + }); + + test('should navigate to Explore when dataset name is clicked', async ({ + page, + }) => { + // Use existing example dataset (hermetic - loaded in CI via --load-examples) + const datasetName = TEST_DATASETS.EXAMPLE_DATASET; + const dataset = await getDatasetByName(page, datasetName); + expect(dataset).not.toBeNull(); + + // Verify dataset is visible in list (uses page object + Playwright auto-wait) + await expect(datasetListPage.getDatasetRow(datasetName)).toBeVisible(); + + // Click on dataset name to navigate to Explore + await datasetListPage.clickDatasetName(datasetName); + + // Wait for Explore page to load (validates URL + datasource control) + await explorePage.waitForPageLoad(); + + // Verify correct dataset is loaded in datasource control + const loadedDatasetName = await explorePage.getDatasetName(); + expect(loadedDatasetName).toContain(datasetName); + + // Verify visualization switcher shows default viz type (indicates full page load) + await expect(explorePage.getVizSwitcher()).toBeVisible(); + await expect(explorePage.getVizSwitcher()).toContainText('Table'); + }); + + test('should delete a dataset with confirmation', async ({ page }) => { + // Get example dataset to duplicate + const originalName = TEST_DATASETS.EXAMPLE_DATASET; + const originalDataset = await getDatasetByName(page, originalName); + expect(originalDataset).not.toBeNull(); + + // Create throwaway copy for deletion (hermetic - uses UI duplication) + const datasetName = `test_delete_${Date.now()}`; Review Comment: The timestamp-based naming pattern `test_delete_${Date.now()}` is used for creating unique dataset names, but this could potentially lead to collisions if tests run in parallel or in rapid succession. While unlikely, this could cause intermittent test failures. Consider using a more robust unique identifier like: ```typescript const datasetName = `test_delete_${Date.now()}_${Math.random().toString(36).substring(7)}`; ``` Or use a UUID library for guaranteed uniqueness. ```suggestion const datasetName = `test_delete_${Date.now()}_${Math.random().toString(36).substring(7)}`; ``` ########## superset-frontend/playwright/global-setup.ts: ########## @@ -0,0 +1,91 @@ +/** + * 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 { + chromium, + FullConfig, + Browser, + BrowserContext, +} from '@playwright/test'; +import { mkdir } from 'fs/promises'; +import { dirname } from 'path'; +import { AuthPage } from './pages/AuthPage'; + +/** + * Global setup function that runs once before all tests. + * Authenticates as admin user and saves the authentication state + * to be reused by tests in the 'chromium' project (E2E tests). + * + * Auth tests (chromium-unauth project) don't use this - they login + * per-test via beforeEach for isolation and simplicity. + */ +async function globalSetup(config: FullConfig) { + // Get baseURL with fallback to default + // FullConfig.use doesn't exist in the type - baseURL is only in projects[0].use + const baseURL = config.projects[0]?.use?.baseURL || 'http://localhost:8088'; Review Comment: The comment states "FullConfig.use doesn't exist in the type - baseURL is only in projects[0].use" which suggests a workaround for a type limitation. However, this could break if the projects array is empty or if the project structure changes. Consider adding a safety check: ```typescript const baseURL = config.projects[0]?.use?.baseURL || config.use?.baseURL || 'http://localhost:8088'; ``` Or document why `projects[0]` is guaranteed to exist in this context. ```suggestion const baseURL = config.projects[0]?.use?.baseURL || config.use?.baseURL || 'http://localhost:8088'; ``` ########## 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; + +test.describe('Dataset List', () => { + let datasetListPage: DatasetListPage; + let explorePage: ExplorePage; + let testResources: { datasetIds: number[] } = { datasetIds: [] }; + + test.beforeEach(async ({ page }) => { + datasetListPage = new DatasetListPage(page); + explorePage = new ExplorePage(page); + testResources = { datasetIds: [] }; // Reset for each test + + // Navigate to dataset list page + await datasetListPage.goto(); + await datasetListPage.waitForTableLoad(); + }); + + test.afterEach(async ({ page }) => { + // Cleanup any resources created during the test + const promises = []; + for (const datasetId of testResources.datasetIds) { + promises.push( + apiDeleteDataset(page, datasetId, { + failOnStatusCode: false, + }).catch(error => { + // Log cleanup failures to avoid silent resource leaks + console.warn( + `[Cleanup] Failed to delete dataset ${datasetId}:`, + String(error), + ); + }), + ); + } + await Promise.all(promises); + }); + + test('should navigate to Explore when dataset name is clicked', async ({ + page, + }) => { + // Use existing example dataset (hermetic - loaded in CI via --load-examples) + const datasetName = TEST_DATASETS.EXAMPLE_DATASET; + const dataset = await getDatasetByName(page, datasetName); + expect(dataset).not.toBeNull(); + + // Verify dataset is visible in list (uses page object + Playwright auto-wait) + await expect(datasetListPage.getDatasetRow(datasetName)).toBeVisible(); + + // Click on dataset name to navigate to Explore + await datasetListPage.clickDatasetName(datasetName); + + // Wait for Explore page to load (validates URL + datasource control) + await explorePage.waitForPageLoad(); + + // Verify correct dataset is loaded in datasource control + const loadedDatasetName = await explorePage.getDatasetName(); + expect(loadedDatasetName).toContain(datasetName); + + // Verify visualization switcher shows default viz type (indicates full page load) + await expect(explorePage.getVizSwitcher()).toBeVisible(); + await expect(explorePage.getVizSwitcher()).toContainText('Table'); + }); + + test('should delete a dataset with confirmation', async ({ page }) => { + // Get example dataset to duplicate + const originalName = TEST_DATASETS.EXAMPLE_DATASET; + const originalDataset = await getDatasetByName(page, originalName); + expect(originalDataset).not.toBeNull(); + + // Create throwaway copy for deletion (hermetic - uses UI duplication) + const datasetName = `test_delete_${Date.now()}`; + + // Verify original dataset is visible in list + await expect(datasetListPage.getDatasetRow(originalName)).toBeVisible(); + + // Set up response intercept to capture duplicate dataset ID + const duplicateResponsePromise = page.waitForResponse( + response => + response.url().includes(`${ENDPOINTS.DATASET}duplicate`) && + response.status() === 201, + ); + + // Click duplicate action button + await datasetListPage.clickDuplicateAction(originalName); + + // Duplicate modal should appear and be ready for interaction + const duplicateModal = new DuplicateDatasetModal(page); + await duplicateModal.waitForReady(); + + // Fill in new dataset name + await duplicateModal.fillDatasetName(datasetName); + + // Click the Duplicate button + await duplicateModal.clickDuplicate(); + + // Get the duplicate dataset ID from response and track immediately + const duplicateResponse = await duplicateResponsePromise; + const duplicateData = await duplicateResponse.json(); + const duplicateId = duplicateData.id; + + // Track duplicate for cleanup immediately (before any operations that could fail) + testResources = { datasetIds: [duplicateId] }; + + // Modal should close + await duplicateModal.waitForHidden(); + + // Refresh page to see new dataset + await datasetListPage.goto(); + await datasetListPage.waitForTableLoad(); + + // Verify dataset is visible in list + await expect(datasetListPage.getDatasetRow(datasetName)).toBeVisible(); + + // Click delete action button + await datasetListPage.clickDeleteAction(datasetName); + + // Delete confirmation modal should appear + const deleteModal = new DeleteConfirmationModal(page); + await deleteModal.waitForVisible(); + + // Type "DELETE" to confirm + await deleteModal.fillConfirmationInput('DELETE'); + + // Click the Delete button + await deleteModal.clickDelete(); + + // Modal should close + await deleteModal.waitForHidden(); + + // Verify success toast appears with correct message + const toast = new Toast(page); + const successToast = toast.getSuccess(); + await expect(successToast).toBeVisible(); + await expect(toast.getMessage()).toContainText('Deleted'); Review Comment: [nitpick] The test verifies the success toast message using `.toContainText('Deleted')` which is a partial match. This could potentially pass with unintended messages like "Deleted (with errors)" or "Partially Deleted". Consider using a more specific assertion: ```typescript await expect(toast.getMessage()).toHaveText(/^Deleted/); ``` Or match the exact expected message if known: ```typescript await expect(toast.getMessage()).toContainText('Deleted 1 dataset'); ``` ########## 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 `waitForTimeout` method with a fixed delay is generally discouraged in Playwright as it makes tests slower and less reliable. Consider using a more efficient polling approach with `waitFor` conditions or reducing the poll interval. The current implementation uses a 500ms sleep between checks, but Playwright's built-in waiting mechanisms are more efficient. If polling is necessary, consider using `page.waitForFunction` or reducing the interval to 100ms for faster detection. ########## superset-frontend/playwright/helpers/api/dataset.ts: ########## @@ -0,0 +1,165 @@ +/** + * 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 { Page, APIResponse } from '@playwright/test'; +import { apiGet, apiPost, apiDelete, ApiRequestOptions } from './requests'; + +export const ENDPOINTS = { + DATASET: 'api/v1/dataset/', +} as const; + +/** + * TypeScript interface for dataset creation API payload + * Provides compile-time safety for required fields + */ +export interface DatasetCreatePayload { + database: number; + catalog: string | null; + schema: string; + table_name: string; +} + +/** + * TypeScript interface for dataset API response + * Represents the shape of dataset data returned from the API + */ +export interface DatasetResult { + id: number; + table_name: string; + sql?: string; + schema?: string; + database: { + id: number; + database_name: string; + }; + owners?: Array<{ id: number }>; + dataset_type?: 'physical' | 'virtual'; +} + +/** + * POST request to create a dataset + * @param page - Playwright page instance (provides authentication context) + * @param requestBody - Dataset configuration object (database, schema, table_name) + * @returns API response from dataset creation + */ +export async function apiPostDataset( + page: Page, + requestBody: DatasetCreatePayload, +): Promise<APIResponse> { + return apiPost(page, ENDPOINTS.DATASET, requestBody); +} + +/** + * Get a dataset by its table name + * @param page - Playwright page instance (provides authentication context) + * @param tableName - The table_name to search for + * @returns Dataset object if found, null if not found + */ +export async function getDatasetByName( + page: Page, + tableName: string, +): Promise<DatasetResult | null> { + // Use Superset's filter API to search by table_name + const filter = { + filters: [ + { + col: 'table_name', + opr: 'eq', + value: tableName, + }, + ], + }; + const queryParam = encodeURIComponent(JSON.stringify(filter)); + const response = await apiGet(page, `${ENDPOINTS.DATASET}?q=${queryParam}`); + + if (!response.ok()) { + return null; + } + + const body = await response.json(); + if (body.result && body.result.length > 0) { + return body.result[0] as DatasetResult; + } + + return null; +} + +/** + * Duplicates a dataset via API and returns the new dataset info + * @param page - Playwright page instance (provides authentication context) + * @param datasetId - ID of dataset to duplicate + * @param newName - Name for the duplicated dataset + * @returns Promise resolving to new dataset info + */ +export async function duplicateDataset( + page: Page, + datasetId: number, + newName: string, +): Promise<DatasetResult> { + const response = await apiPost(page, `${ENDPOINTS.DATASET}duplicate`, { + base_model_id: datasetId, + table_name: newName, + }); + + if (response.status() !== 201) { + throw new Error( + `Failed to duplicate dataset ${datasetId}: expected 201, got ${response.status()}`, Review Comment: The error message when duplication fails only includes the status code but doesn't include the response body which might contain helpful error details from the server. Consider improving the error message: ```typescript if (response.status() !== 201) { const errorBody = await response.text().catch(() => 'Unable to read response body'); throw new Error( `Failed to duplicate dataset ${datasetId}: expected 201, got ${response.status()}. ${errorBody}`, ); } ``` This would provide more context for debugging failed duplications. ```suggestion const errorBody = await response.text().catch(() => 'Unable to read response body'); throw new Error( `Failed to duplicate dataset ${datasetId}: expected 201, got ${response.status()}. ${errorBody}`, ``` ########## 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)); + } + + const currentUrl = await this.page.url(); + throw new Error( + `Login timeout: session cookie did not appear within ${timeout}ms. Current URL: ${currentUrl}`, Review Comment: The error message could be more helpful by including the expected session cookie name ('session'). This would help developers troubleshoot authentication issues more quickly. Suggested improvement: ```typescript throw new Error( `Login timeout: session cookie ('session') did not appear within ${timeout}ms. Current URL: ${currentUrl}`, ); ``` ```suggestion `Login timeout: session cookie ('session') did not appear within ${timeout}ms. Current URL: ${currentUrl}`, ``` ########## superset-frontend/playwright/helpers/api/requests.ts: ########## @@ -0,0 +1,191 @@ +/** + * 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 { Page, APIResponse } from '@playwright/test'; + +export interface ApiRequestOptions { + headers?: Record<string, string>; + params?: Record<string, string>; + failOnStatusCode?: boolean; + allowMissingCsrf?: boolean; +} + +/** + * Get base URL for Referer header + * Reads from environment variable configured in playwright.config.ts + * Preserves full base URL including path prefix (e.g., /app/prefix) + */ +function getBaseUrl(): string { + // Use environment variable which includes path prefix if configured + // This matches playwright.config.ts baseURL setting exactly + return process.env.PLAYWRIGHT_BASE_URL || 'http://localhost:8088'; +} + +interface CsrfResult { + token: string; + error?: string; +} + +/** + * Get CSRF token from the API endpoint + * Superset provides a CSRF token via api/v1/security/csrf_token/ + * The session cookie is automatically included by page.request + */ +async function getCsrfToken(page: Page): Promise<CsrfResult> { + try { + const response = await page.request.get('api/v1/security/csrf_token/', { + failOnStatusCode: false, + }); + + if (!response.ok()) { + return { + token: '', + error: `HTTP ${response.status()} ${response.statusText()}`, + }; + } + + const json = await response.json(); + return { token: json.result || '' }; + } catch (error) { + return { token: '', error: String(error) }; + } +} + +/** + * Build headers for mutation requests (POST, PUT, PATCH, DELETE) + * Includes CSRF token and Referer for Flask-WTF CSRFProtect + */ +async function buildHeaders( + page: Page, + options?: ApiRequestOptions, +): Promise<Record<string, string>> { + const { token: csrfToken, error: csrfError } = await getCsrfToken(page); + const headers: Record<string, string> = { + 'Content-Type': 'application/json', + ...options?.headers, + }; + + // Include CSRF token and Referer for Flask-WTF CSRFProtect + if (csrfToken) { + headers['X-CSRFToken'] = csrfToken; + headers['Referer'] = getBaseUrl(); + } else if (!options?.allowMissingCsrf) { + const errorDetail = csrfError ? ` (${csrfError})` : ''; + throw new Error( + `Missing CSRF token${errorDetail} - mutation requests require authentication. ` + + 'Ensure global authentication completed or test has valid session.', Review Comment: The `allowMissingCsrf` option could be a security risk if used incorrectly. It bypasses CSRF protection which is critical for preventing cross-site request forgery attacks. Consider adding documentation warnings about when this option should be used, or restricting its usage to specific scenarios. Suggested improvement: ```typescript } else if (!options?.allowMissingCsrf) { const errorDetail = csrfError ? ` (${csrfError})` : ''; throw new Error( `Missing CSRF token${errorDetail} - mutation requests require authentication. ` + 'Ensure global authentication completed or test has valid session. ' + 'Note: allowMissingCsrf should only be used for testing unauthenticated endpoints.', ); } ``` ```suggestion 'Ensure global authentication completed or test has valid session. ' + 'Note: allowMissingCsrf should only be used for testing unauthenticated endpoints.', ``` ########## 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; + +test.describe('Dataset List', () => { + let datasetListPage: DatasetListPage; + let explorePage: ExplorePage; + let testResources: { datasetIds: number[] } = { datasetIds: [] }; + + test.beforeEach(async ({ page }) => { + datasetListPage = new DatasetListPage(page); + explorePage = new ExplorePage(page); + testResources = { datasetIds: [] }; // Reset for each test + + // Navigate to dataset list page + await datasetListPage.goto(); + await datasetListPage.waitForTableLoad(); + }); + + test.afterEach(async ({ page }) => { + // Cleanup any resources created during the test + const promises = []; + for (const datasetId of testResources.datasetIds) { + promises.push( + apiDeleteDataset(page, datasetId, { + failOnStatusCode: false, + }).catch(error => { + // Log cleanup failures to avoid silent resource leaks + console.warn( + `[Cleanup] Failed to delete dataset ${datasetId}:`, + String(error), + ); + }), + ); + } + await Promise.all(promises); + }); + + test('should navigate to Explore when dataset name is clicked', async ({ + page, + }) => { + // Use existing example dataset (hermetic - loaded in CI via --load-examples) + const datasetName = TEST_DATASETS.EXAMPLE_DATASET; + const dataset = await getDatasetByName(page, datasetName); + expect(dataset).not.toBeNull(); + + // Verify dataset is visible in list (uses page object + Playwright auto-wait) + await expect(datasetListPage.getDatasetRow(datasetName)).toBeVisible(); + + // Click on dataset name to navigate to Explore + await datasetListPage.clickDatasetName(datasetName); + + // Wait for Explore page to load (validates URL + datasource control) + await explorePage.waitForPageLoad(); + + // Verify correct dataset is loaded in datasource control + const loadedDatasetName = await explorePage.getDatasetName(); + expect(loadedDatasetName).toContain(datasetName); + + // Verify visualization switcher shows default viz type (indicates full page load) + await expect(explorePage.getVizSwitcher()).toBeVisible(); + await expect(explorePage.getVizSwitcher()).toContainText('Table'); + }); + + test('should delete a dataset with confirmation', async ({ page }) => { + // Get example dataset to duplicate + const originalName = TEST_DATASETS.EXAMPLE_DATASET; + const originalDataset = await getDatasetByName(page, originalName); + expect(originalDataset).not.toBeNull(); + + // Create throwaway copy for deletion (hermetic - uses UI duplication) + const datasetName = `test_delete_${Date.now()}`; + + // Verify original dataset is visible in list + await expect(datasetListPage.getDatasetRow(originalName)).toBeVisible(); + + // Set up response intercept to capture duplicate dataset ID + const duplicateResponsePromise = page.waitForResponse( + response => + response.url().includes(`${ENDPOINTS.DATASET}duplicate`) && + response.status() === 201, + ); + + // Click duplicate action button + await datasetListPage.clickDuplicateAction(originalName); + + // Duplicate modal should appear and be ready for interaction + const duplicateModal = new DuplicateDatasetModal(page); + await duplicateModal.waitForReady(); + + // Fill in new dataset name + await duplicateModal.fillDatasetName(datasetName); + + // Click the Duplicate button + await duplicateModal.clickDuplicate(); + + // Get the duplicate dataset ID from response and track immediately + const duplicateResponse = await duplicateResponsePromise; + const duplicateData = await duplicateResponse.json(); + const duplicateId = duplicateData.id; + + // Track duplicate for cleanup immediately (before any operations that could fail) + testResources = { datasetIds: [duplicateId] }; + + // Modal should close + await duplicateModal.waitForHidden(); + + // Refresh page to see new dataset + await datasetListPage.goto(); + await datasetListPage.waitForTableLoad(); + Review Comment: The pattern of creating a page refresh followed by `waitForTableLoad()` is repeated twice in the test file (lines 144-146 and 224-226). This could be extracted into a helper method to reduce duplication and make the intent clearer. Consider adding a method to `DatasetListPage`: ```typescript async refresh(): Promise<void> { await this.goto(); await this.waitForTableLoad(); } ``` This would simplify these calls to just `await datasetListPage.refresh();` ```suggestion await datasetListPage.refresh(); ``` -- 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]
