sadpandajoe commented on code in PR #36196:
URL: https://github.com/apache/superset/pull/36196#discussion_r2583871640
##########
superset-frontend/playwright/pages/AuthPage.ts:
##########
@@ -83,6 +84,62 @@ 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;
Review Comment:
Fixed in 8d9186392b - changed to nullish coalescing (`??`).
##########
superset-frontend/playwright/pages/AuthPage.ts:
##########
@@ -83,6 +84,62 @@ 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();
+
Review Comment:
Fixed in 8d9186392b - timeout now passed to `waitForResponse`.
##########
superset-frontend/playwright/tests/auth/login.spec.ts:
##########
@@ -20,69 +20,74 @@
import { test, expect } from '@playwright/test';
import { AuthPage } from '../../pages/AuthPage';
import { URL } from '../../utils/urls';
+import { TIMEOUT } from '../../utils/constants';
-test.describe('Login view', () => {
- let authPage: AuthPage;
+// Test credentials - can be overridden via environment variables
+const adminUsername = process.env.PLAYWRIGHT_ADMIN_USERNAME || 'admin';
+const adminPassword = process.env.PLAYWRIGHT_ADMIN_PASSWORD || 'general';
- test.beforeEach(async ({ page }: any) => {
- authPage = new AuthPage(page);
- await authPage.goto();
- await authPage.waitForLoginForm();
- });
+/**
+ * Auth/login tests use per-test navigation via beforeEach.
+ * Each test starts fresh on the login page without global authentication.
+ * This follows the Cypress pattern for auth testing - simple and isolated.
+ */
- test('should redirect to login with incorrect username and password', async
({
- page,
- }: any) => {
- // Setup request interception before login attempt
- const loginRequestPromise = authPage.waitForLoginRequest();
+let authPage: AuthPage;
- // Attempt login with incorrect credentials
- await authPage.loginWithCredentials('admin', 'wrongpassword');
+test.beforeEach(async ({ page }) => {
+ // Navigate to login page before each test (ensures clean state)
+ authPage = new AuthPage(page);
+ await authPage.goto();
+ await authPage.waitForLoginForm();
+});
- // Wait for login request and verify response
- const loginResponse = await loginRequestPromise;
- // Failed login returns 401 Unauthorized or 302 redirect to login
- expect([401, 302]).toContain(loginResponse.status());
+test('should redirect to login with incorrect username and password', async ({
+ page,
+}) => {
+ // Setup request interception before login attempt
+ const loginRequestPromise = authPage.waitForLoginRequest();
- // Wait for redirect to complete before checking URL
- await page.waitForURL((url: any) => url.pathname.endsWith('login/'), {
- timeout: 10000,
- });
+ // Attempt login with incorrect credentials
+ await authPage.loginWithCredentials(adminUsername, 'wrongpassword');
Review Comment:
Fixed in 8d9186392b - now uses `'wronguser', 'wrongpassword'`.
##########
superset-frontend/playwright/pages/ExplorePage.ts:
##########
@@ -0,0 +1,88 @@
+/**
+ * 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 { TIMEOUT } from '../utils/constants';
+
+/**
+ * Explore Page object
+ */
+export class ExplorePage {
+ private readonly page: Page;
+
+ private static readonly SELECTORS = {
+ DATASOURCE_CONTROL: '[data-test="datasource-control"]',
+ VIZ_SWITCHER: '[data-test="fast-viz-switcher"]',
+ } as const;
+
+ constructor(page: Page) {
+ this.page = page;
+ }
+
+ /**
+ * Waits for the Explore page to load.
+ * Validates URL contains /explore/ and datasource control is visible.
+ *
+ * @param options - Optional wait options
+ */
+ async waitForPageLoad(options?: { timeout?: number }): Promise<void> {
+ const timeout = options?.timeout || TIMEOUT.PAGE_LOAD;
Review Comment:
Fixed in 8d9186392b - changed to nullish coalescing (`??`).
--
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]
