Copilot commented on code in PR #36196: URL: https://github.com/apache/superset/pull/36196#discussion_r2582843986
########## superset-frontend/playwright/pages/DatasetListPage.ts: ########## @@ -0,0 +1,115 @@ +/** + * 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, Locator } from '@playwright/test'; +import { Table } from '../components/core'; +import { URL } from '../utils/urls'; + +/** + * Dataset List Page object. + */ +export class DatasetListPage { + private readonly page: Page; + private readonly table: Table; + + private static readonly SELECTORS = { + DATASET_LINK: '[data-test="internal-link"]', + DELETE_ACTION: '.action-button svg[data-icon="delete"]', + EXPORT_ACTION: '.action-button svg[data-icon="upload"]', Review Comment: The selector name `EXPORT_ACTION` uses an upload icon (`data-icon="upload"`), which is semantically confusing. Export actions typically use download icons, not upload icons. This could indicate either: 1. The wrong icon is used in the UI (not a test issue) 2. The selector is misnamed and should be `IMPORT_ACTION` or similar Consider verifying which action this actually triggers, and either rename the selector or add a comment explaining why "upload" icon is used for export: ```typescript EXPORT_ACTION: '.action-button svg[data-icon="upload"]', // Note: UI uses upload icon for export ``` ########## 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) Review Comment: [nitpick] The comment says "10s for page transitions (login → welcome, dataset → explore)" but this is actually less than Playwright's default test timeout (30s). Consider clarifying why 10s was chosen instead of using the default, or if this is meant to fail faster than the default timeout. Example improvement: ```typescript PAGE_LOAD: 10000, // 10s for page transitions - fails faster than default 30s timeout ``` ```suggestion PAGE_LOAD: 10000, // 10s for page transitions (login → welcome, dataset → explore) - fails faster than default 30s 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; +/** + * 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.beforeEach(async ({ page }) => { + // Navigate to login page before each test (ensures clean state) + const authPage = new AuthPage(page); + await authPage.goto(); + await authPage.waitForLoginForm(); +}); + +test('should redirect to login with incorrect username and password', async ({ + page, +}) => { + // Create page object (already on login page from beforeEach) + const authPage = new AuthPage(page); + + // Setup request interception before login attempt + const loginRequestPromise = authPage.waitForLoginRequest(); - test.beforeEach(async ({ page }: any) => { - authPage = new AuthPage(page); - await authPage.goto(); - await authPage.waitForLoginForm(); + // Attempt login with incorrect credentials + await authPage.loginWithCredentials('admin', 'wrongpassword'); + + // 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()); + + // Wait for redirect to complete before checking URL + await page.waitForURL(url => url.pathname.endsWith(URL.LOGIN), { + timeout: TIMEOUT.PAGE_LOAD, }); - test('should redirect to login with incorrect username and password', async ({ - page, - }: any) => { - // Setup request interception before login attempt - const loginRequestPromise = authPage.waitForLoginRequest(); + // Verify we stay on login page + const currentUrl = await authPage.getCurrentUrl(); + expect(currentUrl).toContain(URL.LOGIN); - // Attempt login with incorrect credentials - await authPage.loginWithCredentials('admin', 'wrongpassword'); + // Verify error message is shown + const hasError = await authPage.hasLoginError(); + expect(hasError).toBe(true); +}); - // 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 login with correct username and password', async ({ page }) => { + // Create page object (already on login page from beforeEach) + const authPage = new AuthPage(page); - // Wait for redirect to complete before checking URL - await page.waitForURL((url: any) => url.pathname.endsWith('login/'), { - timeout: 10000, - }); + // Setup request interception before login attempt + const loginRequestPromise = authPage.waitForLoginRequest(); - // Verify we stay on login page - const currentUrl = await authPage.getCurrentUrl(); - expect(currentUrl).toContain(URL.LOGIN); + // Login with correct credentials + await authPage.loginWithCredentials('admin', 'general'); Review Comment: Similar to global-setup.ts, the hardcoded credentials should ideally come from environment variables. This is a minor issue for test code, but it's a best practice to avoid hardcoding credentials even in tests: ```typescript const username = process.env.TEST_ADMIN_USERNAME || 'admin'; const password = process.env.TEST_ADMIN_PASSWORD || 'general'; await authPage.loginWithCredentials(username, password); ``` ```suggestion const username = process.env.TEST_ADMIN_USERNAME || 'admin'; const password = process.env.TEST_ADMIN_PASSWORD || 'general'; await authPage.loginWithCredentials(username, password); ``` ########## superset-frontend/playwright.config.ts: ########## @@ -77,10 +80,32 @@ export default defineConfig({ projects: [ { + // Default project - uses global authentication for speed + // E2E tests login once via global-setup.ts and reuse auth state + // Explicitly ignore auth tests (they run in chromium-unauth project) + // Also respect the global experimental testIgnore setting name: 'chromium', + testIgnore: [ + '**/tests/auth/**/*.spec.ts', Review Comment: The comment explains that auth tests are excluded and experimental tests are respected, but the logic for combining the two ignore patterns could be clearer. The spread operator usage might be confusing to developers. Consider adding a clarifying comment: ```typescript testIgnore: [ // Auth tests run in separate chromium-unauth project '**/tests/auth/**/*.spec.ts', // Conditionally ignore experimental based on INCLUDE_EXPERIMENTAL env var ...(process.env.INCLUDE_EXPERIMENTAL ? [] : ['**/experimental/**']), ], ``` ```suggestion '**/tests/auth/**/*.spec.ts', // Conditionally ignore experimental tests based on INCLUDE_EXPERIMENTAL env var ``` ########## superset-frontend/playwright/pages/AuthPage.ts: ########## @@ -83,6 +84,54 @@ 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 + const loginResponse = await this.waitForLoginRequest(); + + // 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 (may take a moment after redirect) + const pollInterval = TIMEOUT.API_POLL_INTERVAL; + while (Date.now() - startTime < timeout) { + const sessionCookie = await this.getSessionCookie(); + if (sessionCookie && sessionCookie.value) { + // Success - session cookie has landed + return; + } + await this.page.waitForTimeout(pollInterval); + } + + throw new Error( + `Login timeout: session cookie did not appear within ${timeout}ms`, Review Comment: The error message doesn't indicate what URL the user is currently on or what the expected redirect location was. This makes debugging timeout issues difficult. Consider including the current URL: ```typescript const currentUrl = this.page.url(); throw new Error( `Login timeout: session cookie did not appear within ${timeout}ms. Current URL: ${currentUrl}` ); ``` ```suggestion const currentUrl = this.page.url(); throw new Error( `Login timeout: session cookie did not appear within ${timeout}ms. Current URL: ${currentUrl}`, ``` ########## superset-frontend/playwright/helpers/api/requests.ts: ########## @@ -0,0 +1,178 @@ +/** + * 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; +} + +/** + * 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(_page: Page): 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'; +} + +/** + * 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<string> { + try { + const response = await page.request.get('api/v1/security/csrf_token/', { + failOnStatusCode: false, + }); + + if (!response.ok()) { + console.warn('[CSRF] Failed to fetch CSRF token:', response.status()); Review Comment: The warning message for CSRF token fetch failure doesn't provide enough context about what the actual HTTP status was or how it impacts the request. Consider including the status code in the warning: ```typescript console.warn(`[CSRF] Failed to fetch CSRF token: ${response.status()} ${response.statusText()}`); ``` ```suggestion console.warn(`[CSRF] Failed to fetch CSRF token: ${response.status()} ${response.statusText()}`); ``` ########## superset-frontend/playwright/global-setup.ts: ########## @@ -0,0 +1,87 @@ +/** + * 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'; + + console.log('[Global Setup] Authenticating as admin user...'); + + let browser: Browser | null = null; + let context: BrowserContext | null = null; + + try { + // Launch browser + browser = await chromium.launch(); + } catch (error) { + console.error('[Global Setup] Failed to launch browser:', error); + throw new Error('Browser launch failed - check Playwright installation'); + } + + try { + context = await browser.newContext({ baseURL }); + const page = await context.newPage(); + + // Use AuthPage to handle login logic (DRY principle) + const authPage = new AuthPage(page); + await authPage.goto(); + await authPage.waitForLoginForm(); + await authPage.loginWithCredentials('admin', 'general'); Review Comment: The hardcoded credentials `'admin'` and `'general'` are used directly in the global setup. While this is acceptable for test environments, consider documenting that these credentials should never be used in production, or loading them from environment variables for better security practices: ```typescript const username = process.env.TEST_ADMIN_USERNAME || 'admin'; const password = process.env.TEST_ADMIN_PASSWORD || 'general'; await authPage.loginWithCredentials(username, password); ``` This makes it easier to use different credentials in different test environments without code changes. ```suggestion // Use environment variables for credentials, fallback to test defaults. // NEVER use these defaults in production environments. const username = process.env.TEST_ADMIN_USERNAME || 'admin'; const password = process.env.TEST_ADMIN_PASSWORD || 'general'; await authPage.loginWithCredentials(username, password); ``` ########## superset-frontend/playwright/pages/AuthPage.ts: ########## @@ -83,6 +84,54 @@ 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 + const loginResponse = await this.waitForLoginRequest(); + + // 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 (may take a moment after redirect) + const pollInterval = TIMEOUT.API_POLL_INTERVAL; + while (Date.now() - startTime < timeout) { + const sessionCookie = await this.getSessionCookie(); + if (sessionCookie && sessionCookie.value) { + // Success - session cookie has landed + return; + } + await this.page.waitForTimeout(pollInterval); + } + + throw new Error( + `Login timeout: session cookie did not appear within ${timeout}ms`, + ); Review Comment: [nitpick] Using a tight polling loop with `page.waitForTimeout()` to check for cookies can be inefficient. Playwright's `waitForFunction` might be more appropriate here, or consider increasing the poll interval since cookie setting typically happens quickly after a redirect. Consider using a more efficient polling mechanism: ```typescript await this.page.waitForFunction( async () => { const cookies = await document.cookie; return cookies.includes('session='); }, { timeout } ); ``` ```suggestion await this.page.waitForFunction( () => document.cookie.includes('session='), { timeout } ); // Success - session cookie has landed ``` ########## superset-frontend/playwright/components/core/Table.ts: ########## @@ -0,0 +1,83 @@ +/** + * 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'; + +/** + * Table component for Superset ListView tables. + */ +export class Table { + private readonly page: Page; + private readonly tableSelector: string; + + private static readonly SELECTORS = { + TABLE_ROW: '[data-test="table-row"]', + }; + + constructor(page: Page, tableSelector = '[data-test="listview-table"]') { + this.page = page; + this.tableSelector = tableSelector; + } + + /** + * Gets the table element locator + */ + get element(): Locator { + return this.page.locator(this.tableSelector); + } + + /** + * Gets a table row by exact text match in the first cell (dataset name column). + * Uses exact match to avoid substring collisions (e.g., 'members_channels_2' vs 'duplicate_members_channels_2_123'). + * @param rowText - Exact text to find in the row's first cell + */ + getRow(rowText: string): Locator { + return this.element.locator(Table.SELECTORS.TABLE_ROW).filter({ + has: this.page.getByRole('cell', { name: rowText, exact: true }), + }); + } + + /** + * Clicks a link within a specific row + * @param rowText - Text to identify the row + * @param linkSelector - Selector for the link within the row + */ + async clickRowLink(rowText: string, linkSelector: string): Promise<void> { + const row = this.getRow(rowText); + await row.locator(linkSelector).click(); + } + + /** + * Waits for the table to be visible + * @param options - Optional wait options + */ + async waitForVisible(options?: { timeout?: number }): Promise<void> { + await this.element.waitFor({ state: 'visible', ...options }); + } + + /** + * Clicks an action button in a row by selector + * @param rowText - Text to identify the row + * @param selector - CSS selector for the action element + */ + async clickRowAction(rowText: string, selector: string): Promise<void> { + const row = this.getRow(rowText); + await row.locator(selector).first().click(); Review Comment: The `clickRowAction()` method uses `.first()` to handle multiple matching selectors, but this could lead to clicking the wrong element if multiple rows match or if there are multiple action buttons with the same selector. Consider adding validation or a more specific selector: ```typescript async clickRowAction(rowText: string, selector: string): Promise<void> { const row = this.getRow(rowText); const actions = row.locator(selector); const count = await actions.count(); if (count > 1) { console.warn(`Multiple actions found for selector ${selector}, clicking first`); } await actions.first().click(); } ``` Or remove `.first()` and let it fail naturally if there are multiple matches, which helps catch selector issues. ```suggestion const actions = row.locator(selector); const count = await actions.count(); if (count === 0) { throw new Error(`No action found for selector "${selector}" in row "${rowText}"`); } if (count > 1) { throw new Error(`Multiple actions (${count}) found for selector "${selector}" in row "${rowText}". Please use a more specific selector.`); } await actions.nth(0).click(); ``` ########## superset-frontend/playwright/tests/experimental/dataset/dataset-list.spec.ts: ########## @@ -0,0 +1,201 @@ +/** + * 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, + duplicateDataset, + ENDPOINTS, +} from '../../../helpers/api/dataset'; + +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(() => {}), + ); + } + 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 = 'members_channels_2'; + 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 originalDataset = await getDatasetByName(page, 'members_channels_2'); + expect(originalDataset).not.toBeNull(); + + // Create throwaway copy for deletion (hermetic - uses duplicate API) + const datasetName = `test_delete_${Date.now()}`; + const duplicateDatasetResult = await duplicateDataset( + page, + originalDataset!.id, + datasetName, + ); + testResources = { datasetIds: [duplicateDatasetResult.id] }; + + // 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'); + + // Verify dataset is removed from list + await expect(datasetListPage.getDatasetRow(datasetName)).not.toBeVisible(); + }); + + test('should duplicate a dataset with new name', async ({ page }) => { + // Use virtual example dataset (members_channels_2) + const originalName = 'members_channels_2'; + const duplicateName = `duplicate_${originalName}_${Date.now()}`; + + // Get the dataset by name (ID varies by environment) + const original = await getDatasetByName(page, originalName); + expect(original).not.toBeNull(); + expect(original!.id).toBeGreaterThan(0); + + // 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(duplicateName); + + // Click the Duplicate button + await duplicateModal.clickDuplicate(); + + // Get the duplicate dataset ID from response + const duplicateResponse = await duplicateResponsePromise; + const duplicateData = await duplicateResponse.json(); + const duplicateId = duplicateData.id; + + // Track duplicate for cleanup (original is example data, don't delete it) + testResources = { datasetIds: [duplicateId] }; + Review Comment: Similar to the issue on line 100, the resource tracking happens after getting the duplicate ID from the response. If any error occurs between lines 169-173 (e.g., network error, timeout), the dataset will be created but not tracked for cleanup. Consider tracking resources earlier or using try-finally: ```typescript let duplicateId: number | null = null; try { const duplicateResponse = await duplicateResponsePromise; const duplicateData = await duplicateResponse.json(); duplicateId = duplicateData.id; testResources.datasetIds.push(duplicateId); // ... rest of test } finally { // cleanup happens in afterEach } ``` ```suggestion // Get the duplicate dataset ID from response and track for cleanup let duplicateId: number | null = null; try { const duplicateResponse = await duplicateResponsePromise; const duplicateData = await duplicateResponse.json(); duplicateId = duplicateData.id; testResources = { datasetIds: [duplicateId] }; } catch (error) { // Optionally log the error or rethrow throw error; } ``` ########## superset-frontend/playwright/global-setup.ts: ########## @@ -0,0 +1,87 @@ +/** + * 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'; + + console.log('[Global Setup] Authenticating as admin user...'); + + let browser: Browser | null = null; + let context: BrowserContext | null = null; + + try { + // Launch browser + browser = await chromium.launch(); + } catch (error) { + console.error('[Global Setup] Failed to launch browser:', error); + throw new Error('Browser launch failed - check Playwright installation'); + } + + try { + context = await browser.newContext({ baseURL }); + const page = await context.newPage(); + + // Use AuthPage to handle login logic (DRY principle) + const authPage = new AuthPage(page); + await authPage.goto(); + await authPage.waitForLoginForm(); + await authPage.loginWithCredentials('admin', 'general'); + await authPage.waitForLoginSuccess(); + + // Save authentication state for all tests to reuse + const authStatePath = 'playwright/.auth/user.json'; + await mkdir(dirname(authStatePath), { recursive: true }); + await context.storageState({ + path: authStatePath, + }); + + console.log( + '[Global Setup] Authentication successful - state saved to playwright/.auth/user.json', + ); + } catch (error) { + console.error('[Global Setup] Authentication failed:', error); + throw error; Review Comment: The error logging doesn't provide helpful debugging information. When authentication fails, the actual error is logged but there's no context about which step failed (navigation, form wait, login credentials, or cookie verification). Consider logging which step failed to help developers debug issues: ```typescript } catch (error) { const step = page.url().includes('login') ? 'during login' : 'navigating to login'; console.error(`[Global Setup] Authentication failed ${step}:`, error); throw error; } ``` ########## superset-frontend/playwright/tests/experimental/dataset/dataset-list.spec.ts: ########## @@ -0,0 +1,201 @@ +/** + * 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, + duplicateDataset, + ENDPOINTS, +} from '../../../helpers/api/dataset'; + +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(() => {}), + ); + } + 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 = 'members_channels_2'; + 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 originalDataset = await getDatasetByName(page, 'members_channels_2'); + expect(originalDataset).not.toBeNull(); + + // Create throwaway copy for deletion (hermetic - uses duplicate API) + const datasetName = `test_delete_${Date.now()}`; + const duplicateDatasetResult = await duplicateDataset( + page, + originalDataset!.id, + datasetName, + ); + testResources = { datasetIds: [duplicateDatasetResult.id] }; Review Comment: The test resource tracking is fragile. If the duplicate operation succeeds but the test fails before line 100, the dataset ID won't be added to `testResources` and cleanup won't happen. This will leave orphaned datasets. Consider tracking the resource immediately after creation: ```typescript const duplicateDatasetResult = await duplicateDataset( page, originalDataset!.id, datasetName, ); // Track immediately to ensure cleanup even if test fails testResources.datasetIds.push(duplicateDatasetResult.id); ``` Then remove line 100. ```suggestion testResources.datasetIds.push(duplicateDatasetResult.id); ``` ########## 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.locator('button', { hasText: buttonText }); Review Comment: The `getFooterButton()` method uses `hasText` which performs a substring match, not an exact match. This could cause issues if multiple buttons contain the search text (e.g., "Delete" and "Delete All"). Consider using `filter` with `hasText` with exact match option, or use `getByRole`: ```typescript private getFooterButton(buttonText: string): Locator { return this.footer.getByRole('button', { name: buttonText, exact: true }); } ``` ```suggestion return this.footer.getByRole('button', { name: buttonText, exact: true }); ``` ########## superset-frontend/playwright/tests/experimental/dataset/dataset-list.spec.ts: ########## @@ -0,0 +1,201 @@ +/** + * 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, + duplicateDataset, + ENDPOINTS, +} from '../../../helpers/api/dataset'; + +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(() => {}), + ); + } + 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 = 'members_channels_2'; Review Comment: [nitpick] The hardcoded dataset name `'members_channels_2'` is used directly in the test. If this dataset doesn't exist in the test environment, the test will fail. Consider documenting this dependency or adding a check to skip the test if the dataset doesn't exist. Alternatively, define example dataset names as constants at the top of the file for easier maintenance: ```typescript const EXAMPLE_DATASETS = { MEMBERS_CHANNELS: 'members_channels_2', } as const; ``` ########## superset-frontend/playwright/helpers/api/dataset.ts: ########## @@ -0,0 +1,160 @@ +/** + * 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, expect } 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, + }); + + expect(response.status()).toBe(201); + const data = await response.json(); + + return { + id: data.id, + table_name: data.result.table_name, + sql: data.result.sql, + schema: data.result.schema, + database: data.result.database, + owners: data.result.owners, + dataset_type: data.result.dataset_type, + }; Review Comment: The response mapping manually extracts fields from `data.result` but the structure differs from other API responses in this file. The inconsistency could lead to bugs if the API response structure changes. Consider documenting why `data.id` and `data.result.*` are accessed differently than in other functions, or create a separate interface for the duplicate response: ```typescript interface DuplicateDatasetResponse { id: number; result: DatasetResult; } ``` ########## superset-frontend/playwright/helpers/api/requests.ts: ########## @@ -0,0 +1,178 @@ +/** + * 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; +} + +/** + * 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(_page: Page): 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'; +} + +/** + * 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<string> { + try { + const response = await page.request.get('api/v1/security/csrf_token/', { + failOnStatusCode: false, + }); + + if (!response.ok()) { + console.warn('[CSRF] Failed to fetch CSRF token:', response.status()); + return ''; + } + + const json = await response.json(); + return json.result || ''; + } catch (error) { + console.warn('[CSRF] Error fetching CSRF token:', error); + return ''; + } +} + +/** + * 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 csrfToken = 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(page); Review Comment: When `csrfToken` is empty or falsy, the CSRF token header and Referer header are not set, but the function continues with the request. This will cause all mutation requests (POST, PUT, PATCH, DELETE) to fail with CSRF validation errors when the CSRF endpoint is unavailable. Consider throwing an error or at least logging a warning when CSRF token is missing for mutation requests: ```typescript if (csrfToken) { headers['X-CSRFToken'] = csrfToken; headers['Referer'] = getBaseUrl(page); } else { console.warn('[CSRF] Missing CSRF token - mutation request may fail'); } ``` ```suggestion headers['Referer'] = getBaseUrl(page); } else { console.warn('[CSRF] Missing CSRF token - mutation request may fail'); ``` ########## .github/workflows/bashlib.sh: ########## @@ -117,6 +117,19 @@ testdata() { say "::endgroup::" } +playwright_testdata() { + cd "$GITHUB_WORKSPACE" + say "::group::Load all examples for Playwright tests" + # must specify PYTHONPATH to make `tests.superset_test_config` importable + export PYTHONPATH="$GITHUB_WORKSPACE" + pip install -e . + superset db upgrade + superset load_test_users + superset load_examples Review Comment: [nitpick] The `playwright_testdata()` function uses `load_examples` which loads all examples into the database. Consider whether this is necessary for all Playwright tests or if a more targeted approach would be faster. Loading all examples can significantly increase test setup time. If only specific datasets are needed (e.g., `members_channels_2`), consider loading only those examples or creating minimal test data via API. ```suggestion say "::group::Load minimal examples for Playwright tests" # must specify PYTHONPATH to make `tests.superset_test_config` importable export PYTHONPATH="$GITHUB_WORKSPACE" pip install -e . superset db upgrade superset load_test_users # Load only the required example dataset(s) for Playwright tests to speed up setup superset load_examples --only members_channels_2 ``` ########## superset-frontend/playwright/tests/experimental/dataset/dataset-list.spec.ts: ########## @@ -0,0 +1,201 @@ +/** + * 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, + duplicateDataset, + ENDPOINTS, +} from '../../../helpers/api/dataset'; + +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(() => {}), + ); Review Comment: The cleanup loop uses `.catch(() => {})` to silently ignore all errors. This makes it difficult to debug cleanup issues. Consider logging errors that occur during cleanup: ```typescript for (const datasetId of testResources.datasetIds) { promises.push( apiDeleteDataset(page, datasetId, { failOnStatusCode: false, }).catch((error) => { console.warn(`Failed to cleanup dataset ${datasetId}:`, error); }), ); } ``` ```suggestion }).catch((error) => { console.warn(`Failed to cleanup dataset ${datasetId}:`, error); }), ``` ########## 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; +/** + * 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.beforeEach(async ({ page }) => { + // Navigate to login page before each test (ensures clean state) + const authPage = new AuthPage(page); + await authPage.goto(); + await authPage.waitForLoginForm(); +}); + +test('should redirect to login with incorrect username and password', async ({ + page, +}) => { + // Create page object (already on login page from beforeEach) + const authPage = new AuthPage(page); Review Comment: [nitpick] Creating a new `AuthPage` instance in each test when one is already created in `beforeEach` on line 33 leads to duplicate instantiation. While functionally correct, this pattern is confusing and wastes resources. Consider either: 1. Make `authPage` available to all tests via a shared variable, or 2. Remove the instantiation from `beforeEach` and only create it in each test Example approach 1: ```typescript let authPage: AuthPage; test.beforeEach(async ({ page }) => { authPage = new AuthPage(page); await authPage.goto(); await authPage.waitForLoginForm(); }); test('should redirect...', async ({ page }) => { // Use authPage directly, no new instantiation needed const loginRequestPromise = authPage.waitForLoginRequest(); // ... }); ``` ########## superset-frontend/playwright/components/core/Table.ts: ########## @@ -0,0 +1,83 @@ +/** + * 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'; + +/** + * Table component for Superset ListView tables. + */ +export class Table { + private readonly page: Page; + private readonly tableSelector: string; + + private static readonly SELECTORS = { + TABLE_ROW: '[data-test="table-row"]', + }; + + constructor(page: Page, tableSelector = '[data-test="listview-table"]') { + this.page = page; + this.tableSelector = tableSelector; + } + + /** + * Gets the table element locator + */ + get element(): Locator { + return this.page.locator(this.tableSelector); + } + + /** + * Gets a table row by exact text match in the first cell (dataset name column). + * Uses exact match to avoid substring collisions (e.g., 'members_channels_2' vs 'duplicate_members_channels_2_123'). + * @param rowText - Exact text to find in the row's first cell + */ + getRow(rowText: string): Locator { + return this.element.locator(Table.SELECTORS.TABLE_ROW).filter({ + has: this.page.getByRole('cell', { name: rowText, exact: true }), + }); + } Review Comment: The documentation mentions that `getRow()` uses exact match to avoid substring collisions and provides a good example. However, it doesn't document what happens when no matching row is found. The method returns a Locator that may not exist in the DOM, which could lead to confusing timeout errors. Consider adding to the documentation: ```typescript /** * Gets a table row by exact text match in the first cell (dataset name column). * Uses exact match to avoid substring collisions (e.g., 'members_channels_2' vs 'duplicate_members_channels_2_123'). * @param rowText - Exact text to find in the row's first cell * @returns Locator for the row (may not exist in DOM - use with expect().toBeVisible() or check visibility) */ ``` ########## superset-frontend/playwright/helpers/api/dataset.ts: ########## @@ -0,0 +1,160 @@ +/** + * 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, expect } 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, + }); + + expect(response.status()).toBe(201); Review Comment: The `duplicateDataset()` function uses `expect()` from Playwright test inside an API helper function. This violates separation of concerns - helper functions should return results or throw errors, not contain test assertions. If this function is called from non-test code or needs to handle non-201 responses differently, it will fail unexpectedly. Consider returning the response and letting the caller decide how to handle it: ```typescript 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: ${response.status()}`); } const data = await response.json(); return { // ... mapping }; } ``` ```suggestion if (response.status() !== 201) { throw new Error(`Failed to duplicate dataset: ${response.status()}`); } ``` -- 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]
