sadpandajoe commented on code in PR #36196:
URL: https://github.com/apache/superset/pull/36196#discussion_r2583889512
##########
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:
Error message already includes 'session cookie' context. The specific cookie
name is an implementation detail - error clearly indicates what's expected.
##########
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 catch block logs the actual error with full stack trace. Adding step
context would require wrapping each call, which adds complexity for marginal
benefit in this simple setup.
##########
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:
Already addressed - tests now use response interception to track IDs
immediately after API response, before any operations that could fail.
##########
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 duplicate endpoint returns a different response structure than other
endpoints. The mapping is intentional and documented in the function's return
type.
--
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]
