sadpandajoe commented on code in PR #36196:
URL: https://github.com/apache/superset/pull/36196#discussion_r2583873418
##########
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:
Already addressed - now uses shared `let authPage: AuthPage` variable
created in `beforeEach`.
##########
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:
Already addressed - now throws `Error` instead of warning when CSRF token is
missing (with `allowMissingCsrf` escape hatch for edge cases).
##########
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:
Already addressed - now uses `getByRole('button', { name, 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(() => {}),
+ );
Review Comment:
Already addressed - cleanup errors are now logged with `console.warn` and
`String(error)` for robustness.
--
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]
