From 8952e80ffd6a3fb7c5f32eeeaf6be015da91c319 Mon Sep 17 00:00:00 2001 From: Joe Li Date: Tue, 11 Nov 2025 11:35:28 -0800 Subject: [PATCH] refactor(playwright): reorganize dataset tests to experimental directory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After rebasing onto master, reorganize Playwright tests to follow the experimental pattern that was adopted in master: - Move dataset E2E tests to tests/experimental/ directory - Update import paths to reflect new directory structure (../../ → ../../../) - Update chromium project testIgnore to respect experimental pattern - Add comprehensive README explaining experimental test workflow - Auth tests remain stable (always run by default) - Dataset tests are now opt-in (require INCLUDE_EXPERIMENTAL=true) Test organization: - tests/auth/ - Stable auth tests (2 tests) - tests/experimental/dataset/ - Experimental dataset tests (2 tests) All supporting infrastructure remains in stable locations: - playwright/components/ - Modal, Table, Toast, etc. - playwright/pages/ - AuthPage, DatasetListPage, ExplorePage - playwright/helpers/api/ - Full CRUD + factories Verification: - Without INCLUDE_EXPERIMENTAL: 2 tests (auth only) - With INCLUDE_EXPERIMENTAL=true: 4 tests (auth + dataset) --- superset-frontend/playwright.config.ts | 6 +- .../playwright/tests/experimental/README.md | 128 ++++++++++++------ .../dataset/dataset-list.spec.ts | 14 +- 3 files changed, 99 insertions(+), 49 deletions(-) rename superset-frontend/playwright/tests/{ => experimental}/dataset/dataset-list.spec.ts (89%) diff --git a/superset-frontend/playwright.config.ts b/superset-frontend/playwright.config.ts index 18368000b4c..935aa731c63 100644 --- a/superset-frontend/playwright.config.ts +++ b/superset-frontend/playwright.config.ts @@ -83,8 +83,12 @@ export default defineConfig({ // Default project - uses global authentication for speed // E2E tests login once via global-setup.ts and reuse auth state // Explicitly ignore auth tests (they run in chromium-unauth project) + // Also respect the global experimental testIgnore setting name: 'chromium', - testIgnore: '**/tests/auth/**/*.spec.ts', + testIgnore: [ + '**/tests/auth/**/*.spec.ts', + ...(process.env.INCLUDE_EXPERIMENTAL ? [] : ['**/experimental/**']), + ], use: { browserName: 'chromium', testIdAttribute: 'data-test', diff --git a/superset-frontend/playwright/tests/experimental/README.md b/superset-frontend/playwright/tests/experimental/README.md index 9647fb23960..ac70dd1f6e9 100644 --- a/superset-frontend/playwright/tests/experimental/README.md +++ b/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. -## Promoting Tests to Stable +## How Experimental Tests Work -Once a test has proven stable (no false positives/negatives over sufficient time): - -1. Move the test file out of `experimental/` to the appropriate feature directory: - ```bash - # From the repository root: - git mv superset-frontend/playwright/tests/experimental/dashboard/test.spec.ts \ - superset-frontend/playwright/tests/dashboard/ - - # Or from the superset-frontend/ directory: - git mv playwright/tests/experimental/dashboard/test.spec.ts \ - playwright/tests/dashboard/ - ``` - -2. The test will automatically become required for merge - -## Test Organization - -Organize tests by feature area: -- `auth/` - Authentication and authorization tests -- `dashboard/` - Dashboard functionality tests -- `explore/` - Chart builder tests -- `sqllab/` - SQL Lab tests -- etc. - -## Running Tests +### Running Tests +**By default (CI and local), experimental tests are EXCLUDED:** ```bash -# Run all experimental tests (requires INCLUDE_EXPERIMENTAL env var) -INCLUDE_EXPERIMENTAL=true npm run playwright:test -- experimental/ - -# Run specific experimental test -INCLUDE_EXPERIMENTAL=true npm run playwright:test -- experimental/dashboard/test.spec.ts - -# Run in UI mode for debugging -INCLUDE_EXPERIMENTAL=true npm run playwright:ui -- experimental/ +npm run playwright:test +# Only runs stable tests (tests/auth/*) ``` -**Note**: The `INCLUDE_EXPERIMENTAL=true` environment variable is required because experimental tests are filtered out by default in `playwright.config.ts`. Without it, Playwright will report "No tests found". +**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 + +- **Experimental CI jobs** (optional): Use `TEST_PATH=experimental/` + - `.github/workflows/bashlib.sh` sets `INCLUDE_EXPERIMENTAL=true` when `TEST_PATH` is provided + - These jobs can use `continue-on-error: true` for shadow mode + +### Configuration + +The experimental pattern is configured in `playwright.config.ts`: + +```typescript +testIgnore: process.env.INCLUDE_EXPERIMENTAL + ? undefined + : '**/experimental/**', +``` + +This ensures: +- Without `INCLUDE_EXPERIMENTAL`: Tests in `experimental/` are ignored +- With `INCLUDE_EXPERIMENTAL=true`: All tests run, including experimental + +## When to Use Experimental + +Add tests to `experimental/` when: + +1. **Testing new infrastructure** - New page objects, components, or patterns that need real-world validation +2. **Flaky tests** - Tests that pass locally but have intermittent CI failures that need investigation +3. **New test types** - E2E tests for new features that need to prove stability before becoming required +4. **Prototyping** - Experimental approaches that may or may not become standard patterns + +## Moving Tests to Stable + +Once an experimental test has proven stable (consistent CI passes over time): + +1. **Move the test file** from `experimental/` to the appropriate stable directory: + ```bash + git mv tests/experimental/dataset/my-test.spec.ts tests/dataset/my-test.spec.ts + ``` + +2. **Commit the move** with a clear message: + ```bash + git commit -m "test(playwright): promote my-test from experimental to stable" + ``` + +3. **Test will now be required** - It will run by default and block merges on failure + +## Current Experimental Tests + +### Dataset Tests + +- **`dataset/dataset-list.spec.ts`** - Dataset list E2E tests + - Status: Infrastructure complete, validating stability + - Includes: Delete dataset test with API-based test data + - Supporting infrastructure: API helpers, Modal components, page objects + +## Infrastructure Location + +**Important**: Supporting infrastructure (components, page objects, API helpers) should live in **stable locations**, NOT under `experimental/`: + +✅ **Correct locations:** +- `playwright/components/` - Components used by any tests +- `playwright/pages/` - Page objects for any features +- `playwright/helpers/api/` - API helpers for test data setup + +❌ **Avoid:** +- `playwright/tests/experimental/components/` - Makes it hard to share infrastructure + +This keeps infrastructure reusable and avoids duplication when tests graduate from experimental to stable. + +## Questions? + +See [Superset Testing Documentation](https://superset.apache.org/docs/contributing/development#testing) or ask in the `#testing` Slack channel. diff --git a/superset-frontend/playwright/tests/dataset/dataset-list.spec.ts b/superset-frontend/playwright/tests/experimental/dataset/dataset-list.spec.ts similarity index 89% rename from superset-frontend/playwright/tests/dataset/dataset-list.spec.ts rename to superset-frontend/playwright/tests/experimental/dataset/dataset-list.spec.ts index f2a1a8dd024..11783bf0dca 100644 --- a/superset-frontend/playwright/tests/dataset/dataset-list.spec.ts +++ b/superset-frontend/playwright/tests/experimental/dataset/dataset-list.spec.ts @@ -18,13 +18,13 @@ */ import { test, expect, Page } from '@playwright/test'; -import { DatasetListPage } from '../../pages/DatasetListPage'; -import { ExplorePage } from '../../pages/ExplorePage'; -import { DeleteConfirmationModal } from '../../components/modals/DeleteConfirmationModal'; -import { Toast } from '../../components/core/Toast'; -import { createTestDataset } from '../../helpers/api/dataset.factories'; -import { apiDeleteDataset } from '../../helpers/api/dataset'; -import { apiDeleteDatabase } from '../../helpers/api/database'; +import { DatasetListPage } from '../../../pages/DatasetListPage'; +import { ExplorePage } from '../../../pages/ExplorePage'; +import { DeleteConfirmationModal } from '../../../components/modals/DeleteConfirmationModal'; +import { Toast } from '../../../components/core/Toast'; +import { createTestDataset } from '../../../helpers/api/dataset.factories'; +import { apiDeleteDataset } from '../../../helpers/api/dataset'; +import { apiDeleteDatabase } from '../../../helpers/api/database'; test.describe('Dataset List', () => { let datasetListPage: DatasetListPage;