korbit-ai[bot] commented on code in PR #35110: URL: https://github.com/apache/superset/pull/35110#discussion_r2345334965
########## superset-frontend/playwright.config.ts: ########## @@ -0,0 +1,90 @@ +/** + * 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. + */ + +/// <reference types="node" /> + +// eslint-disable-next-line import/no-extraneous-dependencies +import { defineConfig } from '@playwright/test'; + +export default defineConfig({ + // Test directory + testDir: './playwright/tests', + + // Timeout settings + timeout: 30000, + expect: { timeout: 8000 }, + + // Parallel execution + fullyParallel: true, + workers: process.env.CI ? 2 : 1, + + // Retry logic - 2 retries in CI, 0 locally + retries: process.env.CI ? 2 : 0, + + // Reporter configuration - multiple reporters for better visibility + reporter: process.env.CI + ? [ + ['github'], // GitHub Actions annotations + ['list'], // Detailed output with summary table + ['html', { outputFolder: 'playwright-report', open: 'never' }], // Interactive report + ['json', { outputFile: 'test-results/results.json' }], // Machine-readable + ] + : [ + ['list'], // Shows summary table locally + ['html', { outputFolder: 'playwright-report', open: 'on-failure' }], // Auto-open on failure + ], + + // Global test setup + use: { + // Use environment variable for base URL in CI, default to localhost:8088 for local + baseURL: process.env.PLAYWRIGHT_BASE_URL || 'http://localhost:8088', + + // Browser settings + headless: !!process.env.CI, + + viewport: { width: 1280, height: 1024 }, + + // Screenshots and videos on failure + screenshot: 'only-on-failure', + video: 'retain-on-failure', + + // Trace collection for debugging + trace: 'retain-on-failure', + }, + + projects: [ + { + name: 'chromium', + use: { + browserName: 'chromium', + testIdAttribute: 'data-test', + }, + }, + ], + + // Web server setup - disabled in CI (Flask started separately in workflow) + webServer: process.env.CI + ? undefined + : { + command: 'curl -f http://localhost:8088/health', Review Comment: ### Incorrect webServer Command <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The webServer command only checks if the server is reachable but doesn't actually start the development server. ###### Why this matters Tests will fail locally as there's no server running to test against. The health check alone doesn't initialize the required test environment. ###### Suggested change ∙ *Feature Preview* Replace the command with the actual command to start the development server: ```typescript command: 'npm run dev', ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/00c80044-840b-4c4a-b4e0-5c4ab5d159f3/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/00c80044-840b-4c4a-b4e0-5c4ab5d159f3?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/00c80044-840b-4c4a-b4e0-5c4ab5d159f3?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/00c80044-840b-4c4a-b4e0-5c4ab5d159f3?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/00c80044-840b-4c4a-b4e0-5c4ab5d159f3) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:785e2bed-1034-493c-9cfa-76e6921c6614 --> [](785e2bed-1034-493c-9cfa-76e6921c6614) ########## superset-frontend/playwright/pages/AuthPage.ts: ########## @@ -0,0 +1,122 @@ +/** + * 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, Response } from '@playwright/test'; +import { Form } from '../components/core'; +import { URL } from '../utils/urls'; + +export class AuthPage { + private readonly page: Page; + + private readonly loginForm: Form; + + // Selectors specific to the auth/login page + private static readonly SELECTORS = { + LOGIN_FORM: '[data-test="login-form"]', + USERNAME_INPUT: '[data-test="username-input"]', + PASSWORD_INPUT: '[data-test="password-input"]', + LOGIN_BUTTON: '[data-test="login-button"]', + ERROR_SELECTORS: [ + '[role="alert"]', + '.ant-form-item-explain-error', + '.ant-form-item-explain.ant-form-item-explain-error', + '.alert-danger', + ], + } as const; + + constructor(page: Page) { + this.page = page; + this.loginForm = new Form(page, AuthPage.SELECTORS.LOGIN_FORM); + } + + /** + * Navigate to the login page + */ + async goto(): Promise<void> { + await this.page.goto(URL.LOGIN); + } + + /** + * Wait for login form to be visible + */ + async waitForLoginForm(): Promise<void> { + await this.loginForm.waitForVisible({ timeout: 5000 }); + } + + /** + * Login with provided credentials + * @param username - Username to enter + * @param password - Password to enter + */ + async loginWithCredentials( + username: string, + password: string, + ): Promise<void> { + const usernameInput = this.loginForm.getInput( + AuthPage.SELECTORS.USERNAME_INPUT, + ); + const passwordInput = this.loginForm.getInput( + AuthPage.SELECTORS.PASSWORD_INPUT, + ); + const loginButton = this.loginForm.getButton( + AuthPage.SELECTORS.LOGIN_BUTTON, + ); + + await usernameInput.fill(username); + await passwordInput.fill(password); + await loginButton.click(); + } + + /** + * Get current page URL + */ + async getCurrentUrl(): Promise<string> { + return this.page.url(); + } + + /** + * Get the session cookie specifically + */ + async getSessionCookie(): Promise<{ name: string; value: string } | null> { + const cookies = await this.page.context().cookies(); + return cookies.find((c: any) => c.name === 'session') || null; Review Comment: ### Unsafe Cookie Type Checking <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The cookie type is explicitly set to 'any', which bypasses type checking for cookie properties and could lead to runtime errors. ###### Why this matters If the cookie structure changes in future Playwright versions, the code won't catch potential incompatibilities at compile time, potentially causing test failures. ###### Suggested change ∙ *Feature Preview* Use the proper Playwright cookie type: ```typescript import { Cookie } from '@playwright/test'; async getSessionCookie(): Promise<Cookie | null> { const cookies = await this.page.context().cookies(); return cookies.find((c: Cookie) => c.name === 'session') || null; } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fece8598-341d-40f5-8af7-09489944767d/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fece8598-341d-40f5-8af7-09489944767d?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fece8598-341d-40f5-8af7-09489944767d?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fece8598-341d-40f5-8af7-09489944767d?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fece8598-341d-40f5-8af7-09489944767d) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:e1c1387b-8143-4fdb-923f-8ddcd4b5bd79 --> [](e1c1387b-8143-4fdb-923f-8ddcd4b5bd79) ########## superset-frontend/playwright/pages/AuthPage.ts: ########## @@ -0,0 +1,122 @@ +/** + * 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, Response } from '@playwright/test'; +import { Form } from '../components/core'; +import { URL } from '../utils/urls'; + +export class AuthPage { + private readonly page: Page; + + private readonly loginForm: Form; + + // Selectors specific to the auth/login page + private static readonly SELECTORS = { + LOGIN_FORM: '[data-test="login-form"]', + USERNAME_INPUT: '[data-test="username-input"]', + PASSWORD_INPUT: '[data-test="password-input"]', + LOGIN_BUTTON: '[data-test="login-button"]', + ERROR_SELECTORS: [ + '[role="alert"]', + '.ant-form-item-explain-error', + '.ant-form-item-explain.ant-form-item-explain-error', + '.alert-danger', + ], + } as const; + + constructor(page: Page) { + this.page = page; + this.loginForm = new Form(page, AuthPage.SELECTORS.LOGIN_FORM); + } + + /** + * Navigate to the login page + */ + async goto(): Promise<void> { + await this.page.goto(URL.LOGIN); + } + + /** + * Wait for login form to be visible + */ + async waitForLoginForm(): Promise<void> { + await this.loginForm.waitForVisible({ timeout: 5000 }); + } Review Comment: ### Insufficient Login Form Timeout <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The hardcoded timeout of 5000ms for the login form visibility check may be insufficient in slower environments or under heavy load. ###### Why this matters Under slow network conditions or system load, the login form might take longer than 5 seconds to appear, causing false test failures. ###### Suggested change ∙ *Feature Preview* Make the timeout configurable with a default value that's more accommodating: ```typescript async waitForLoginForm(timeout = 30000): Promise<void> { await this.loginForm.waitForVisible({ timeout }); } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ba830c92-153c-4269-b16e-4535563e8520/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ba830c92-153c-4269-b16e-4535563e8520?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ba830c92-153c-4269-b16e-4535563e8520?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ba830c92-153c-4269-b16e-4535563e8520?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ba830c92-153c-4269-b16e-4535563e8520) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:07d4a8fa-2bfc-4b7b-ad61-dff1a4585060 --> [](07d4a8fa-2bfc-4b7b-ad61-dff1a4585060) ########## superset-frontend/playwright/components/core/Form.ts: ########## @@ -0,0 +1,110 @@ +/** + * 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'; +import { Input } from './Input'; +import { Button } from './Button'; + +export class Form { + private readonly page: Page; + + private readonly locator: Locator; + + constructor(page: Page, selector: string); + + constructor(page: Page, locator: Locator); + + constructor(page: Page, selectorOrLocator: string | Locator) { + this.page = page; + if (typeof selectorOrLocator === 'string') { + this.locator = page.locator(selectorOrLocator); + } else { + this.locator = selectorOrLocator; + } + } + + /** + * Gets the form element locator + */ + get element(): Locator { + return this.locator; + } + + /** + * Gets an input field within the form (properly scoped) + * @param inputSelector - Selector for the input field + */ + getInput(inputSelector: string): Input { + const scopedLocator = this.locator.locator(inputSelector); + return new Input(this.page, scopedLocator); + } + + /** + * Gets a button within the form (properly scoped) + * @param buttonSelector - Selector for the button + */ + getButton(buttonSelector: string): Button { + const scopedLocator = this.locator.locator(buttonSelector); + return new Button(this.page, scopedLocator); + } + + /** + * Checks if the form is visible + */ + async isVisible(): Promise<boolean> { + return this.locator.isVisible(); + } + + /** + * Submits the form (triggers submit event) + */ + async submit(): Promise<void> { + await this.locator.evaluate((form: HTMLElement) => { + if (form instanceof HTMLFormElement) { + form.submit(); + } + }); Review Comment: ### Form Submit Method Bypasses Validation <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The form.submit() method bypasses form validation and doesn't trigger submit event handlers, which could lead to missed test scenarios that depend on form validation or submit event handlers. ###### Why this matters When testing forms in a real application, bypassing form validation and event handlers means the tests won't catch validation errors or execute crucial submit event logic that would occur in actual user interactions. ###### Suggested change ∙ *Feature Preview* Replace the form.submit() with a more realistic form submission approach that triggers validation and events: ```typescript async submit(): Promise<void> { await this.locator.evaluate((form: HTMLElement) => { if (form instanceof HTMLFormElement) { const submitEvent = new Event('submit', { bubbles: true, cancelable: true }); const eventResult = form.dispatchEvent(submitEvent); if (eventResult) { form.submit(); } } }); } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d1a2696e-76af-4bb1-a063-966b88e9d800/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d1a2696e-76af-4bb1-a063-966b88e9d800?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d1a2696e-76af-4bb1-a063-966b88e9d800?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d1a2696e-76af-4bb1-a063-966b88e9d800?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d1a2696e-76af-4bb1-a063-966b88e9d800) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:a42f812e-5fcf-4d0d-9d52-b1d3ba68e5ed --> [](a42f812e-5fcf-4d0d-9d52-b1d3ba68e5ed) ########## .github/workflows/bashlib.sh: ########## @@ -182,6 +182,76 @@ kill $flaskProcessId } +playwright-install() { + cd "$GITHUB_WORKSPACE/superset-frontend" + + say "::group::Install Playwright browsers" + npx playwright install --with-deps chromium + # Create output directories for test results and debugging + mkdir -p playwright-results + mkdir -p test-results + say "::endgroup::" +} + +playwright-run() { + local APP_ROOT=$1 + + # Start Flask from the project root (same as Cypress) + cd "$GITHUB_WORKSPACE" + local flasklog="${HOME}/flask-playwright.log" + local port=8081 + PLAYWRIGHT_BASE_URL="http://localhost:${port}"; + if [ -n "$APP_ROOT" ]; then + export SUPERSET_APP_ROOT=$APP_ROOT + PLAYWRIGHT_BASE_URL=${PLAYWRIGHT_BASE_URL}${APP_ROOT}/ + fi + export PLAYWRIGHT_BASE_URL + + nohup flask run --no-debugger -p $port >"$flasklog" 2>&1 </dev/null & + local flaskProcessId=$! + + # Ensure cleanup on exit + trap "kill $flaskProcessId 2>/dev/null || true" EXIT Review Comment: ### Late Process Cleanup Registration <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The Flask server cleanup trap is set after the server process is started, creating a small window where the process might not be cleaned up if the script fails between these lines. ###### Why this matters If a failure occurs between starting the Flask server and setting the trap, the Flask process could remain running, causing port conflicts in subsequent test runs. ###### Suggested change ∙ *Feature Preview* Move the trap before starting the Flask server: ```bash trap "kill $flaskProcessId 2>/dev/null || true" EXIT nohup flask run --no-debugger -p $port >"$flasklog" 2>&1 </dev/null & local flaskProcessId=$! ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6f24d658-e62b-47fe-80fc-94df66b95961/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6f24d658-e62b-47fe-80fc-94df66b95961?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6f24d658-e62b-47fe-80fc-94df66b95961?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6f24d658-e62b-47fe-80fc-94df66b95961?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6f24d658-e62b-47fe-80fc-94df66b95961) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:bbab22da-a699-4c4f-9474-f1ffbd35ac9d --> [](bbab22da-a699-4c4f-9474-f1ffbd35ac9d) ########## .github/workflows/bashlib.sh: ########## @@ -182,6 +182,76 @@ cypress-run-all() { kill $flaskProcessId } +playwright-install() { + cd "$GITHUB_WORKSPACE/superset-frontend" + + say "::group::Install Playwright browsers" + npx playwright install --with-deps chromium + # Create output directories for test results and debugging + mkdir -p playwright-results + mkdir -p test-results + say "::endgroup::" +} + +playwright-run() { + local APP_ROOT=$1 + + # Start Flask from the project root (same as Cypress) + cd "$GITHUB_WORKSPACE" + local flasklog="${HOME}/flask-playwright.log" + local port=8081 + PLAYWRIGHT_BASE_URL="http://localhost:${port}"; + if [ -n "$APP_ROOT" ]; then + export SUPERSET_APP_ROOT=$APP_ROOT + PLAYWRIGHT_BASE_URL=${PLAYWRIGHT_BASE_URL}${APP_ROOT}/ + fi + export PLAYWRIGHT_BASE_URL + + nohup flask run --no-debugger -p $port >"$flasklog" 2>&1 </dev/null & + local flaskProcessId=$! + + # Ensure cleanup on exit + trap "kill $flaskProcessId 2>/dev/null || true" EXIT + + # Wait for server to be ready with health check + local timeout=60 + say "Waiting for Flask server to start on port $port..." + while [ $timeout -gt 0 ]; do + if curl -f ${PLAYWRIGHT_BASE_URL}/health >/dev/null 2>&1; then + say "Flask server is ready" + break + fi + sleep 1 + timeout=$((timeout - 1)) + done Review Comment: ### Inefficient Health Check Polling <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The health check polling uses a fixed 1-second sleep interval, which can lead to unnecessary waits. ###### Why this matters In scenarios where the server starts quickly, the code still waits a full second before the next check, introducing latency into the test startup. ###### Suggested change ∙ *Feature Preview* Implement an exponential backoff strategy starting with a shorter interval (e.g., 0.1s) that increases up to 1s. Example: ```bash interval=0.1 while [ $timeout -gt 0 ]; do if curl -f ${PLAYWRIGHT_BASE_URL}/health >/dev/null 2>&1; then say "Flask server is ready" break fi sleep $interval interval=$(echo "if($interval < 1.0) $interval * 2 else 1.0" | bc) timeout=$((timeout - 1)) done ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c34cae11-6542-4bd8-a4f1-797da893cf36/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c34cae11-6542-4bd8-a4f1-797da893cf36?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c34cae11-6542-4bd8-a4f1-797da893cf36?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c34cae11-6542-4bd8-a4f1-797da893cf36?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c34cae11-6542-4bd8-a4f1-797da893cf36) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:24daea13-4ced-4d03-a74f-a6eeeb3d456e --> [](24daea13-4ced-4d03-a74f-a6eeeb3d456e) ########## .github/workflows/bashlib.sh: ########## @@ -182,6 +182,76 @@ kill $flaskProcessId } +playwright-install() { + cd "$GITHUB_WORKSPACE/superset-frontend" + + say "::group::Install Playwright browsers" + npx playwright install --with-deps chromium + # Create output directories for test results and debugging + mkdir -p playwright-results + mkdir -p test-results + say "::endgroup::" +} + +playwright-run() { + local APP_ROOT=$1 + + # Start Flask from the project root (same as Cypress) + cd "$GITHUB_WORKSPACE" + local flasklog="${HOME}/flask-playwright.log" + local port=8081 + PLAYWRIGHT_BASE_URL="http://localhost:${port}"; + if [ -n "$APP_ROOT" ]; then + export SUPERSET_APP_ROOT=$APP_ROOT + PLAYWRIGHT_BASE_URL=${PLAYWRIGHT_BASE_URL}${APP_ROOT}/ + fi + export PLAYWRIGHT_BASE_URL + + nohup flask run --no-debugger -p $port >"$flasklog" 2>&1 </dev/null & + local flaskProcessId=$! + + # Ensure cleanup on exit + trap "kill $flaskProcessId 2>/dev/null || true" EXIT + + # Wait for server to be ready with health check + local timeout=60 + say "Waiting for Flask server to start on port $port..." + while [ $timeout -gt 0 ]; do + if curl -f ${PLAYWRIGHT_BASE_URL}/health >/dev/null 2>&1; then + say "Flask server is ready" + break + fi + sleep 1 + timeout=$((timeout - 1)) + done + + if [ $timeout -eq 0 ]; then + echo "::error::Flask server failed to start within 60 seconds" + echo "::group::Flask startup log" + cat "$flasklog" + echo "::endgroup::" + return 1 + fi + + # Change to frontend directory for Playwright execution + cd "$GITHUB_WORKSPACE/superset-frontend" + + say "::group::Run Playwright tests" + echo "Running Playwright with baseURL: ${PLAYWRIGHT_BASE_URL}" + npx playwright test auth/login --reporter=github --output=playwright-results Review Comment: ### Hardcoded Test Suite Path <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The Playwright test command is hardcoded to only run the 'auth/login' test suite, which doesn't align with the developer's intent to migrate existing Cypress tests. ###### Why this matters This limitation prevents running other test suites and effectively blocks the migration of other Cypress tests to Playwright. ###### Suggested change ∙ *Feature Preview* Modify the command to accept a test path parameter: ```bash local test_path=${1:-"auth/login"} npx playwright test $test_path --reporter=github --output=playwright-results ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/30fba1b2-42f5-4497-aecd-eca30314144d/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/30fba1b2-42f5-4497-aecd-eca30314144d?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/30fba1b2-42f5-4497-aecd-eca30314144d?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/30fba1b2-42f5-4497-aecd-eca30314144d?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/30fba1b2-42f5-4497-aecd-eca30314144d) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:303bdef1-bbb1-45bd-ab29-c8f43d8fb2fc --> [](303bdef1-bbb1-45bd-ab29-c8f43d8fb2fc) ########## .github/workflows/bashlib.sh: ########## @@ -182,6 +182,76 @@ kill $flaskProcessId } +playwright-install() { + cd "$GITHUB_WORKSPACE/superset-frontend" + + say "::group::Install Playwright browsers" + npx playwright install --with-deps chromium + # Create output directories for test results and debugging + mkdir -p playwright-results + mkdir -p test-results + say "::endgroup::" +} + +playwright-run() { + local APP_ROOT=$1 + + # Start Flask from the project root (same as Cypress) + cd "$GITHUB_WORKSPACE" + local flasklog="${HOME}/flask-playwright.log" + local port=8081 + PLAYWRIGHT_BASE_URL="http://localhost:${port}"; + if [ -n "$APP_ROOT" ]; then + export SUPERSET_APP_ROOT=$APP_ROOT + PLAYWRIGHT_BASE_URL=${PLAYWRIGHT_BASE_URL}${APP_ROOT}/ + fi + export PLAYWRIGHT_BASE_URL + + nohup flask run --no-debugger -p $port >"$flasklog" 2>&1 </dev/null & + local flaskProcessId=$! + + # Ensure cleanup on exit + trap "kill $flaskProcessId 2>/dev/null || true" EXIT + + # Wait for server to be ready with health check + local timeout=60 + say "Waiting for Flask server to start on port $port..." + while [ $timeout -gt 0 ]; do + if curl -f ${PLAYWRIGHT_BASE_URL}/health >/dev/null 2>&1; then + say "Flask server is ready" + break + fi + sleep 1 + timeout=$((timeout - 1)) + done + + if [ $timeout -eq 0 ]; then + echo "::error::Flask server failed to start within 60 seconds" + echo "::group::Flask startup log" + cat "$flasklog" + echo "::endgroup::" + return 1 + fi + + # Change to frontend directory for Playwright execution + cd "$GITHUB_WORKSPACE/superset-frontend" + + say "::group::Run Playwright tests" + echo "Running Playwright with baseURL: ${PLAYWRIGHT_BASE_URL}" + npx playwright test auth/login --reporter=github --output=playwright-results + local status=$? + say "::endgroup::" + + # After job is done, print out Flask log for debugging + echo "::group::Flask log for Playwright run" + cat "$flasklog" + echo "::endgroup::" + # make sure the program exits + kill $flaskProcessId + + return $status +} Review Comment: ### Inflexible Playwright Test Configuration <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The Playwright test configuration is hardcoded in the function, limiting flexibility and reusability. ###### Why this matters Hardcoded test parameters make it difficult to run different test suites or configure test runs differently without modifying the script. ###### Suggested change ∙ *Feature Preview* Parameterize the test configuration: ```bash playwright-run() { local APP_ROOT=$1 local TEST_SPEC=${2:-"auth/login"} local REPORTER=${3:-"github"} local OUTPUT_DIR=${4:-"playwright-results"} npx playwright test "$TEST_SPEC" --reporter="$REPORTER" --output="$OUTPUT_DIR" } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7cec4873-84c6-4e38-8fb9-c8bd9f6283bd/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7cec4873-84c6-4e38-8fb9-c8bd9f6283bd?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7cec4873-84c6-4e38-8fb9-c8bd9f6283bd?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7cec4873-84c6-4e38-8fb9-c8bd9f6283bd?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7cec4873-84c6-4e38-8fb9-c8bd9f6283bd) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:5658fda7-64bd-4921-b028-a5e28a82c74d --> [](5658fda7-64bd-4921-b028-a5e28a82c74d) ########## superset-frontend/playwright/pages/AuthPage.ts: ########## @@ -0,0 +1,122 @@ +/** + * 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, Response } from '@playwright/test'; +import { Form } from '../components/core'; +import { URL } from '../utils/urls'; + +export class AuthPage { + private readonly page: Page; + + private readonly loginForm: Form; + + // Selectors specific to the auth/login page + private static readonly SELECTORS = { + LOGIN_FORM: '[data-test="login-form"]', + USERNAME_INPUT: '[data-test="username-input"]', + PASSWORD_INPUT: '[data-test="password-input"]', + LOGIN_BUTTON: '[data-test="login-button"]', + ERROR_SELECTORS: [ + '[role="alert"]', + '.ant-form-item-explain-error', + '.ant-form-item-explain.ant-form-item-explain-error', + '.alert-danger', + ], + } as const; + + constructor(page: Page) { + this.page = page; + this.loginForm = new Form(page, AuthPage.SELECTORS.LOGIN_FORM); + } + + /** + * Navigate to the login page + */ + async goto(): Promise<void> { + await this.page.goto(URL.LOGIN); + } + + /** + * Wait for login form to be visible + */ + async waitForLoginForm(): Promise<void> { + await this.loginForm.waitForVisible({ timeout: 5000 }); + } + + /** + * Login with provided credentials + * @param username - Username to enter + * @param password - Password to enter + */ + async loginWithCredentials( + username: string, + password: string, + ): Promise<void> { + const usernameInput = this.loginForm.getInput( + AuthPage.SELECTORS.USERNAME_INPUT, + ); + const passwordInput = this.loginForm.getInput( + AuthPage.SELECTORS.PASSWORD_INPUT, + ); + const loginButton = this.loginForm.getButton( + AuthPage.SELECTORS.LOGIN_BUTTON, + ); + + await usernameInput.fill(username); + await passwordInput.fill(password); + await loginButton.click(); + } + + /** + * Get current page URL + */ + async getCurrentUrl(): Promise<string> { + return this.page.url(); + } + + /** + * Get the session cookie specifically + */ + async getSessionCookie(): Promise<{ name: string; value: string } | null> { + const cookies = await this.page.context().cookies(); + return cookies.find((c: any) => c.name === 'session') || null; + } + + /** + * Check if login form has validation errors + */ + async hasLoginError(): Promise<boolean> { + const visibilityPromises = AuthPage.SELECTORS.ERROR_SELECTORS.map( + selector => this.page.locator(selector).isVisible(), + ); + const visibilityResults = await Promise.all(visibilityPromises); + return visibilityResults.some((isVisible: any) => isVisible); + } Review Comment: ### Error detection logic should be encapsulated in Form component <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The method contains inline implementation of error detection logic that could be abstracted into the Form class for better reusability and separation of concerns. ###### Why this matters This creates tight coupling between AuthPage and error detection implementation details. If error detection logic needs to change, it would require modifications in multiple page objects instead of a single Form class. ###### Suggested change ∙ *Feature Preview* Move error detection logic to Form class: ```typescript // In Form class async hasErrors(errorSelectors: string[]): Promise<boolean> { const visibilityPromises = errorSelectors.map( selector => this.page.locator(selector).isVisible() ); const visibilityResults = await Promise.all(visibilityPromises); return visibilityResults.some(isVisible => isVisible); } // In AuthPage async hasLoginError(): Promise<boolean> { return this.loginForm.hasErrors(AuthPage.SELECTORS.ERROR_SELECTORS); } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0250a9f5-7989-4a29-8c34-a28c94dcaa62/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0250a9f5-7989-4a29-8c34-a28c94dcaa62?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0250a9f5-7989-4a29-8c34-a28c94dcaa62?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0250a9f5-7989-4a29-8c34-a28c94dcaa62?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0250a9f5-7989-4a29-8c34-a28c94dcaa62) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:cd85c157-543b-4e23-963c-507788730eeb --> [](cd85c157-543b-4e23-963c-507788730eeb) ########## superset-frontend/playwright/pages/AuthPage.ts: ########## @@ -0,0 +1,122 @@ +/** + * 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, Response } from '@playwright/test'; +import { Form } from '../components/core'; +import { URL } from '../utils/urls'; + +export class AuthPage { + private readonly page: Page; + + private readonly loginForm: Form; + + // Selectors specific to the auth/login page + private static readonly SELECTORS = { + LOGIN_FORM: '[data-test="login-form"]', + USERNAME_INPUT: '[data-test="username-input"]', + PASSWORD_INPUT: '[data-test="password-input"]', + LOGIN_BUTTON: '[data-test="login-button"]', + ERROR_SELECTORS: [ + '[role="alert"]', + '.ant-form-item-explain-error', + '.ant-form-item-explain.ant-form-item-explain-error', + '.alert-danger', + ], + } as const; + + constructor(page: Page) { + this.page = page; + this.loginForm = new Form(page, AuthPage.SELECTORS.LOGIN_FORM); + } + + /** + * Navigate to the login page + */ + async goto(): Promise<void> { + await this.page.goto(URL.LOGIN); + } + + /** + * Wait for login form to be visible + */ + async waitForLoginForm(): Promise<void> { + await this.loginForm.waitForVisible({ timeout: 5000 }); + } + + /** + * Login with provided credentials + * @param username - Username to enter + * @param password - Password to enter + */ + async loginWithCredentials( + username: string, + password: string, + ): Promise<void> { + const usernameInput = this.loginForm.getInput( + AuthPage.SELECTORS.USERNAME_INPUT, + ); + const passwordInput = this.loginForm.getInput( + AuthPage.SELECTORS.PASSWORD_INPUT, + ); + const loginButton = this.loginForm.getButton( + AuthPage.SELECTORS.LOGIN_BUTTON, + ); + + await usernameInput.fill(username); + await passwordInput.fill(password); + await loginButton.click(); + } + + /** + * Get current page URL + */ + async getCurrentUrl(): Promise<string> { + return this.page.url(); + } + + /** + * Get the session cookie specifically + */ + async getSessionCookie(): Promise<{ name: string; value: string } | null> { + const cookies = await this.page.context().cookies(); + return cookies.find((c: any) => c.name === 'session') || null; + } + + /** + * Check if login form has validation errors + */ + async hasLoginError(): Promise<boolean> { + const visibilityPromises = AuthPage.SELECTORS.ERROR_SELECTORS.map( + selector => this.page.locator(selector).isVisible(), + ); + const visibilityResults = await Promise.all(visibilityPromises); + return visibilityResults.some((isVisible: any) => isVisible); + } + + /** + * Wait for a login request to be made and return the response + */ + async waitForLoginRequest(): Promise<Response> { + return this.page.waitForResponse( + (response: any) => + response.url().includes('/login/') && + response.request().method() === 'POST', Review Comment: ### Untyped Response Handling <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The response parameter is typed as 'any' in the waitForLoginRequest method, which could mask potential API response handling issues. ###### Why this matters Without proper typing, changes in the Playwright Response interface might not be caught during development, leading to potential runtime failures. ###### Suggested change ∙ *Feature Preview* Use the proper Playwright Response type: ```typescript (response: Response) => response.url().includes('/login/') && response.request().method() === 'POST' ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/510c4d83-e147-4c7e-97b2-0f1cedf3875a/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/510c4d83-e147-4c7e-97b2-0f1cedf3875a?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/510c4d83-e147-4c7e-97b2-0f1cedf3875a?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/510c4d83-e147-4c7e-97b2-0f1cedf3875a?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/510c4d83-e147-4c7e-97b2-0f1cedf3875a) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:c8a39d84-a27e-4826-9d2b-06b038224568 --> [](c8a39d84-a27e-4826-9d2b-06b038224568) -- 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: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
