Copilot commented on code in PR #36196: URL: https://github.com/apache/superset/pull/36196#discussion_r2583642029
########## superset-frontend/playwright/helpers/api/requests.ts: ########## @@ -0,0 +1,191 @@ +/** + * 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; + allowMissingCsrf?: 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 { Review Comment: The `_page` parameter is declared but never used in this function. The underscore prefix suggests this was intentional to avoid unused variable warnings, but the parameter itself appears unnecessary since the base URL is retrieved from an environment variable. Consider removing the parameter entirely if it's not needed: ```typescript function getBaseUrl(): string { return process.env.PLAYWRIGHT_BASE_URL || 'http://localhost:8088'; } ``` This would make the function signature clearer and eliminate the need for the underscore prefix. ########## superset-frontend/playwright/tests/experimental/README.md: ########## @@ -19,52 +19,98 @@ under the License. # Experimental Playwright Tests -This directory contains Playwright tests that are still under development or validation. - ## Purpose -Tests in this directory run in "shadow mode" with `continue-on-error: true` in CI: -- Failures do NOT block PR merges -- Allows tests to run in CI to validate stability before promotion -- Provides visibility into test reliability over time +This directory contains **experimental** Playwright E2E tests that are being developed and stabilized before becoming part of the required test suite. + +## How Experimental Tests Work + +### Running Tests + +**By default (CI and local), experimental tests are EXCLUDED:** +```bash +npm run playwright:test +# Only runs stable tests (tests/auth/*) +``` + +**To include experimental tests, set the environment variable:** +```bash +INCLUDE_EXPERIMENTAL=true npm run playwright:test +# Runs all tests including experimental/ +``` + +### CI Behavior + +- **Required CI jobs**: Experimental tests are excluded by default + - Tests in `experimental/` do NOT block merges + - Failures in `experimental/` do NOT fail the build -## Promoting Tests to Stable +- **Experimental CI jobs** (optional): Use `TEST_PATH=experimental/` + - `.github/workflows/bashlib.sh` sets `INCLUDE_EXPERIMENTAL=true` when `TEST_PATH` is provided Review Comment: The documentation states that `.github/workflows/bashlib.sh` sets `INCLUDE_EXPERIMENTAL=true` when `TEST_PATH` is provided, but this behavior is not implemented in the actual bashlib.sh file. The `playwright_testdata()` function doesn't set this environment variable. Either update the documentation to remove this claim, or implement the described behavior if it's needed for the experimental CI jobs. ```suggestion - Set `INCLUDE_EXPERIMENTAL=true` in the job environment to include experimental tests ``` -- 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]
