fix tour / whats new collision

2026-02-12 04:28:29 -08:00
parent ee15c67429
commit 4d323a51ca
9 changed files with 2618 additions and 1 deletions
--- a/docs/adr/0058-browser-test-performance-optimization.md
+++ b/docs/adr/0058-browser-test-performance-optimization.md
@@ -0,0 +1,517 @@
+# ADR-0042: Browser Test Performance Optimization
+
+**Status**: Accepted
+**Date**: 2026-02-10
+**Authors**: Claude Code AI Agent
+
+## Context
+
+### Current State
+
+The stock-alert project has 64 Playwright browser tests across 5 spec files taking approximately 240 seconds (~4 minutes) to execute. Analysis reveals three major performance bottlenecks:
+
+| Metric                              | Count | Impact                                       |
+| ----------------------------------- | ----- | -------------------------------------------- |
+| Hardcoded `waitForTimeout()` calls  | 66    | ~120s cumulative wait time                   |
+| Redundant login calls per test      | 43    | ~2-3s each = 86-129s overhead                |
+| Visual regression tests blocking CI | 4     | Cannot run in parallel with functional tests |
+
+### Test Distribution
+
+| File                | Tests  | `waitForTimeout` Calls | `login()` Calls          |
+| ------------------- | ------ | ---------------------- | ------------------------ |
+| `dashboard.spec.js` | 10     | 8                      | 10                       |
+| `alerts.spec.js`    | 14     | 25                     | 1 (beforeEach)           |
+| `gaps.spec.js`      | 20     | 29                     | 1 (beforeEach)           |
+| `login.spec.js`     | 11     | 4                      | 0 (tests login itself)   |
+| `visual.spec.js`    | 4      | 0                      | 4 (via navigateWithAuth) |
+| **Total**           | **59** | **66**                 | **16 patterns**          |
+
+### Root Causes
+
+1. **Anti-Pattern: Hardcoded Timeouts**
+   - `waitForTimeout(2000)` used to "wait for data to load"
+   - Unnecessarily slow on fast systems, flaky on slow systems
+   - No correlation to actual page readiness
+
+2. **Anti-Pattern: Per-Test Authentication**
+   - Each test navigates to `/login`, enters password, submits
+   - Session cookie persists across requests but not across tests
+   - `beforeEach` login adds 2-3 seconds per test
+
+3. **Architecture: Mixed Test Types**
+   - Visual regression tests require different infrastructure (baseline images)
+   - Functional tests and visual tests compete for worker slots
+   - Cannot optimize CI parallelization
+
+### Requirements
+
+1. Reduce test suite runtime by 40-55%
+2. Improve test determinism (eliminate flakiness)
+3. Maintain test coverage and reliability
+4. Enable parallel CI execution where possible
+5. Document patterns for other projects
+
+## Decision
+
+Implement three optimization phases:
+
+### Phase 1: Event-Based Wait Replacement (Primary Impact: ~50% of time savings)
+
+Replace all 66 `waitForTimeout()` calls with Playwright's event-based waiting APIs.
+
+**Replacement Patterns:**
+
+| Current Pattern                         | Replacement                                       | Rationale                     |
+| --------------------------------------- | ------------------------------------------------- | ----------------------------- |
+| `waitForTimeout(2000)` after navigation | `waitForLoadState('networkidle')`                 | Waits for network quiescence  |
+| `waitForTimeout(1000)` after click      | `waitForSelector('.result')`                      | Waits for specific DOM change |
+| `waitForTimeout(3000)` for charts       | `waitForSelector('canvas', { state: 'visible' })` | Waits for chart render        |
+| `waitForTimeout(500)` for viewport      | `waitForFunction(() => ...)`                      | Waits for layout reflow       |
+
+**Implementation Examples:**
+
+```javascript
+// BEFORE: Hardcoded timeout
+await page.goto('/alerts');
+await page.waitForTimeout(2000);
+const rows = await page.locator('tbody tr').count();
+
+// AFTER: Event-based wait
+await page.goto('/alerts');
+await page.waitForLoadState('networkidle');
+await page.waitForSelector('tbody tr', { state: 'attached' });
+const rows = await page.locator('tbody tr').count();
+```
+
+```javascript
+// BEFORE: Hardcoded timeout after action
+await page.click('#runCheckBtn');
+await page.waitForTimeout(2000);
+
+// AFTER: Wait for response
+const [response] = await Promise.all([
+  page.waitForResponse((resp) => resp.url().includes('/api/check')),
+  page.click('#runCheckBtn'),
+]);
+```
+
+**Helper Function Addition to `helpers.js`:**
+
+```javascript
+/**
+ * Waits for page to be fully loaded with data.
+ * Replaces hardcoded waitForTimeout calls.
+ */
+async function waitForPageReady(page, options = {}) {
+  const { dataSelector = null, networkIdle = true, minTime = 0 } = options;
+
+  const promises = [];
+
+  if (networkIdle) {
+    promises.push(page.waitForLoadState('networkidle'));
+  }
+
+  if (dataSelector) {
+    promises.push(page.waitForSelector(dataSelector, { state: 'visible' }));
+  }
+
+  if (minTime > 0) {
+    promises.push(page.waitForTimeout(minTime)); // Escape hatch for animations
+  }
+
+  await Promise.all(promises);
+}
+```
+
+**Estimated Time Savings:** 60-80 seconds (eliminates ~120s of cumulative waits, but event waits have overhead)
+
+### Phase 2: Global Authentication Setup (Primary Impact: ~35% of time savings)
+
+Share authenticated session across all tests using Playwright's global setup feature.
+
+**Architecture:**
+
+```
+                         ┌──────────────────┐
+                         │  global-setup.js │
+                         │                  │
+                         │ 1. Login once    │
+                         │ 2. Save storage  │
+                         └────────┬─────────┘
+                                  │
+           ┌──────────────────────┼──────────────────────┐
+           │                      │                      │
+           ▼                      ▼                      ▼
+  ┌─────────────────┐   ┌─────────────────┐   ┌─────────────────┐
+  │ dashboard.spec  │   │  alerts.spec    │   │   gaps.spec     │
+  │ (reuses auth)   │   │ (reuses auth)   │   │ (reuses auth)   │
+  └─────────────────┘   └─────────────────┘   └─────────────────┘
+```
+
+**Implementation Files:**
+
+**`tests/browser/global-setup.js`:**
+
+```javascript
+const { chromium } = require('@playwright/test');
+const path = require('path');
+
+const authFile = path.join(__dirname, '.auth', 'user.json');
+
+module.exports = async function globalSetup() {
+  const browser = await chromium.launch();
+  const page = await browser.newPage();
+
+  // Only perform login if authentication is enabled
+  if (process.env.DASHBOARD_PASSWORD) {
+    await page.goto(process.env.PLAYWRIGHT_BASE_URL || 'http://localhost:8980');
+
+    // Perform login
+    await page.goto('/login');
+    await page.fill('#password', process.env.DASHBOARD_PASSWORD);
+    await page.click('button[type="submit"]');
+    await page.waitForURL('/');
+
+    // Save authentication state
+    await page.context().storageState({ path: authFile });
+  }
+
+  await browser.close();
+};
+```
+
+**`playwright.config.js` Updates:**
+
+```javascript
+module.exports = defineConfig({
+  // ... existing config ...
+
+  // Global setup runs once before all tests
+  globalSetup: require.resolve('./tests/browser/global-setup.js'),
+
+  projects: [
+    {
+      name: 'chromium',
+      use: {
+        ...devices['Desktop Chrome'],
+        // Reuse authentication state from global setup
+        storageState: './tests/browser/.auth/user.json',
+      },
+    },
+  ],
+});
+```
+
+**Test File Updates:**
+
+```javascript
+// BEFORE: Login in beforeEach
+test.beforeEach(async ({ page }) => {
+  page.consoleErrors = captureConsoleErrors(page);
+  if (isAuthEnabled()) {
+    await login(page);
+  }
+});
+
+// AFTER: Remove login (handled by global setup)
+test.beforeEach(async ({ page }) => {
+  page.consoleErrors = captureConsoleErrors(page);
+  // Authentication already applied via storageState
+});
+```
+
+**Estimated Time Savings:** 80-100 seconds (43 logins x ~2-3s each, minus 3s for global setup)
+
+### Phase 3: Visual Test Separation (Primary Impact: CI parallelization)
+
+Separate visual regression tests into a dedicated project for parallel CI execution.
+
+**Project Configuration:**
+
+```javascript
+// playwright.config.js
+module.exports = defineConfig({
+  projects: [
+    // Functional tests - fast, event-based
+    {
+      name: 'functional',
+      testMatch: /^(?!.*visual).*\.spec\.js$/,
+      use: {
+        ...devices['Desktop Chrome'],
+        storageState: './tests/browser/.auth/user.json',
+      },
+    },
+    // Visual tests - separate baseline management
+    {
+      name: 'visual',
+      testMatch: '**/visual.spec.js',
+      use: {
+        ...devices['Desktop Chrome'],
+        storageState: './tests/browser/.auth/user.json',
+      },
+      // Different snapshot handling
+      snapshotPathTemplate: '{testDir}/__screenshots__/{projectName}/{testFilePath}/{arg}{ext}',
+    },
+  ],
+});
+```
+
+**CI Pipeline Updates:**
+
+```yaml
+# .gitea/workflows/test.yml
+jobs:
+  browser-functional:
+    runs-on: ubuntu-latest
+    steps:
+      - run: npx playwright test --project=functional
+
+  browser-visual:
+    runs-on: ubuntu-latest
+    steps:
+      - run: npx playwright test --project=visual
+```
+
+**Estimated Time Savings:** 30-45 seconds (parallel execution vs sequential)
+
+## Implementation Schedule
+
+### Critical Path (Estimated 8-12 hours)
+
+```
+Phase 1 (Event Waits)      ████████████████ [4-6h]
+                                          │
+Phase 2 (Global Auth)                     ████████ [2-3h]
+                                                  │
+Phase 3 (Visual Separation)                       ████ [2-3h]
+```
+
+### Effort Summary
+
+| Phase                    | Min Hours | Max Hours | Expected Savings      |
+| ------------------------ | --------- | --------- | --------------------- |
+| 1. Event-Based Waits     | 4         | 6         | 60-80s (25-33%)       |
+| 2. Global Authentication | 2         | 3         | 80-100s (33-42%)      |
+| 3. Visual Separation     | 2         | 3         | 30-45s (CI parallel)  |
+| **Total**                | **8**     | **12**    | **170-225s (70-94%)** |
+
+### Expected Results
+
+| Metric             | Before | After     | Improvement   |
+| ------------------ | ------ | --------- | ------------- |
+| Total Runtime      | 240s   | 110-140s  | 42-54% faster |
+| Flaky Test Rate    | ~5%    | <1%       | 80% reduction |
+| CI Parallelization | None   | 2 workers | 2x throughput |
+| Login Operations   | 43     | 1         | 98% reduction |
+| Hardcoded Waits    | 66     | <5        | 92% reduction |
+
+## Consequences
+
+### Positive
+
+1. **Performance**: 40-55% reduction in test runtime
+2. **Reliability**: Event-based waits eliminate timing flakiness
+3. **Scalability**: Global setup pattern scales to N tests with O(1) login cost
+4. **CI Efficiency**: Parallel visual tests enable faster feedback loops
+5. **Maintainability**: Centralized auth logic reduces code duplication
+6. **Transferable Knowledge**: Patterns applicable to any Playwright project
+
+### Negative
+
+1. **Initial Migration Effort**: 8-12 hours of refactoring
+2. **Learning Curve**: Team must understand Playwright wait APIs
+3. **Global Setup Complexity**: Adds shared state between tests
+4. **Debugging Harder**: Shared auth can mask test isolation issues
+
+### Mitigations
+
+| Risk               | Mitigation                                                    |
+| ------------------ | ------------------------------------------------------------- |
+| Global setup fails | Add retry logic; fallback to per-test login                   |
+| Event waits flaky  | Keep small timeout buffer (100ms) as escape hatch             |
+| Visual tests drift | Separate baseline management per environment                  |
+| Test isolation     | Run `--project=functional` without auth for isolation testing |
+
+### Neutral
+
+- Test count unchanged (59 tests)
+- Coverage unchanged
+- Visual baselines unchanged (path changes only)
+
+## Alternatives Considered
+
+### Alternative 1: Reduce Test Count
+
+**Rejected:** Sacrifices coverage for speed. Tests exist for a reason.
+
+### Alternative 2: Increase Worker Parallelism
+
+**Rejected:** Server cannot handle >2 concurrent sessions reliably; creates resource contention.
+
+### Alternative 3: Use `page.waitForTimeout()` with Shorter Durations
+
+**Rejected:** Addresses symptom, not root cause. Still creates timing-dependent tests.
+
+### Alternative 4: Cookie Injection Instead of Login
+
+**Rejected:** Requires reverse-engineering session format; brittle if auth changes.
+
+### Alternative 5: HTTP API Authentication (No Browser)
+
+**Rejected:** Loses browser session behavior validation; tests login flow.
+
+## Implementation Details
+
+### Wait Replacement Mapping
+
+| File                | Current Timeouts       | Replacement Strategy                                                   |
+| ------------------- | ---------------------- | ---------------------------------------------------------------------- |
+| `dashboard.spec.js` | 1000ms, 2000ms, 3000ms | `waitForSelector` for charts, `waitForLoadState` for navigation        |
+| `alerts.spec.js`    | 500ms, 1000ms, 2000ms  | `waitForResponse` for API calls, `waitForSelector` for table rows      |
+| `gaps.spec.js`      | 500ms, 1000ms, 2000ms  | `waitForResponse` for `/api/gaps`, `waitForSelector` for summary cards |
+| `login.spec.js`     | 500ms, 2000ms          | `waitForURL` for redirects, `waitForSelector` for error messages       |
+
+### Common Wait Patterns for This Codebase
+
+| Scenario              | Recommended Pattern                               | Example                   |
+| --------------------- | ------------------------------------------------- | ------------------------- |
+| After page navigation | `waitForLoadState('networkidle')`                 | Loading dashboard data    |
+| After button click    | `waitForResponse()` + `waitForSelector()`         | Run Check button          |
+| After filter change   | `waitForResponse(/api\/.*/)`                      | Status filter dropdown    |
+| For chart rendering   | `waitForSelector('canvas', { state: 'visible' })` | Chart cards               |
+| For modal appearance  | `waitForSelector('.modal', { state: 'visible' })` | Confirmation dialogs      |
+| For layout change     | `waitForFunction()`                               | Responsive viewport tests |
+
+### Auth Storage Structure
+
+```
+tests/browser/
+├── .auth/
+│   └── user.json          # Generated by global-setup, gitignored
+├── global-setup.js        # Creates user.json
+├── dashboard.spec.js      # Uses storageState
+├── alerts.spec.js
+├── gaps.spec.js
+├── login.spec.js          # Tests login itself, may need special handling
+└── visual.spec.js
+```
+
+**`.gitignore` Addition:**
+
+```
+tests/browser/.auth/
+```
+
+### Login.spec.js Special Handling
+
+`login.spec.js` tests the login flow itself and must NOT use the shared auth state:
+
+```javascript
+// playwright.config.js
+projects: [
+  {
+    name: 'functional',
+    testMatch: /^(?!.*login).*\.spec\.js$/,
+    use: { storageState: './tests/browser/.auth/user.json' },
+  },
+  {
+    name: 'login',
+    testMatch: '**/login.spec.js',
+    use: { storageState: undefined }, // No auth - tests login flow
+  },
+];
+```
+
+## Testing the Optimization
+
+### Baseline Measurement
+
+```bash
+# Before optimization: establish baseline
+time npm run test:browser 2>&1 | tee baseline-timing.log
+grep -E "passed|failed|skipped" baseline-timing.log
+```
+
+### Incremental Verification
+
+```bash
+# After Phase 1: verify wait replacement
+npm run test:browser -- --reporter=list 2>&1 | grep -E "passed|failed|slow"
+
+# After Phase 2: verify global auth
+npm run test:browser -- --trace on
+# Check trace for login occurrences (should be 1)
+
+# After Phase 3: verify parallel execution
+npm run test:browser -- --project=functional &
+npm run test:browser -- --project=visual &
+wait
+```
+
+### Success Criteria
+
+| Metric                 | Target | Measurement                            |
+| ---------------------- | ------ | -------------------------------------- |
+| Total runtime          | <150s  | `time npm run test:browser`            |
+| Login count            | 1      | Grep traces for `/login` navigation    |
+| Flaky rate             | <2%    | 50 consecutive CI runs                 |
+| `waitForTimeout` count | <5     | `grep -r waitForTimeout tests/browser` |
+
+## Lessons Learned / Patterns for Other Projects
+
+### Pattern 1: Always Prefer Event-Based Waits
+
+```javascript
+// Bad
+await page.click('#submit');
+await page.waitForTimeout(2000);
+expect(await page.title()).toBe('Success');
+
+// Good
+await Promise.all([page.waitForNavigation(), page.click('#submit')]);
+expect(await page.title()).toBe('Success');
+```
+
+### Pattern 2: Global Setup for Authentication
+
+Playwright's `storageState` feature should be the default for any authenticated app:
+
+1. Create `global-setup.js` that performs login once
+2. Save cookies/storage to JSON file
+3. Configure `storageState` in `playwright.config.js`
+4. Tests start authenticated with zero overhead
+
+### Pattern 3: Separate Test Types by Execution Characteristics
+
+| Test Type  | Characteristics          | Strategy                          |
+| ---------- | ------------------------ | --------------------------------- |
+| Functional | Fast, deterministic      | Run first, gate deployment        |
+| Visual     | Slow, baseline-dependent | Run in parallel, separate project |
+| E2E        | Cross-service, slow      | Run nightly, separate workflow    |
+
+### Pattern 4: Measure Before and After
+
+Always establish baseline metrics before optimization:
+
+```bash
+# Essential metrics to capture
+time npm run test:browser       # Total runtime
+grep -c waitForTimeout *.js     # Hardcoded wait count
+grep -c 'await login' *.js      # Login call count
+```
+
+## Related ADRs
+
+- [ADR-0031](0031-quality-gates-eslint-playwright.md): Quality Gates - ESLint, Pre-commit Hooks, and Playwright Browser Testing
+- [ADR-0035](0035-browser-test-selector-fixes.md): Browser Test Selector Fixes
+- [ADR-0008](0008-testing-strategy.md): Testing Strategy
+
+## References
+
+- Playwright Best Practices: https://playwright.dev/docs/best-practices
+- Playwright Authentication: https://playwright.dev/docs/auth
+- Playwright Wait Strategies: https://playwright.dev/docs/actionability
+- Test Files: `tests/browser/*.spec.js`
+- Helper Module: `tests/browser/helpers.js`
+- Configuration: `playwright.config.js`