♻️ Migrate storybook tests to node:test

[Robdel12]

Summary

• Migrate @vizzly-testing/storybook test suite from Vitest to Node's built-in test runner (node:test)
• Port Puppeteer CI optimizations from @vizzly-testing/static-site
• Improve test coverage from 87% to 99%

Changes

Test Migration:

• Replace vitest imports with node:test and node:assert/strict
• Rename test files from .spec.js to .test.js
• Port vi.fn() mocks to node:test mock helpers
• Update package.json test scripts to use node --test
• Remove vitest dependency and vitest.config.js

Puppeteer Performance (ported from static-site):

• Add CI_OPTIMIZED_ARGS for reduced memory usage in CI environments
• Add protocolTimeout: 60_000 for faster failure detection
• Add navigation timeout fallback from networkidle2 to domcontentloaded

New Tests:

• viewport.test.js - parseViewport, formatViewport, setViewport, getCommonViewports
• patterns.test.js - matchPattern, filterByPattern, findMatchingHook edge cases
• Expanded screenshot.test.js - captureScreenshot, captureAndSendScreenshot
• Expanded crawler.test.js - readIndexJson, discoverStories, generateStoryUrl validation

Test Plan

• All 104 tests pass with npm test
• Coverage at 99.33% lines, 92.86% branches, 97.06% functions
• Lint passes with npm run lint
• No vitest dependency or config remains

Closes #142

[claude]

Code Review

I've reviewed PR #157 and have the following feedback:

---

✅ Strengths

1. Test Migration Quality

• Clean migration from Vitest to Node.js built-in test runner
• Proper use of node:test and node:assert/strict APIs
• Mock functions correctly ported from vi.fn() to mock.fn()
• Assertion style matches Node.js conventions well

2. Improved Test Coverage

• Excellent coverage increase: 87% → 99.33% lines
• New test files provide comprehensive edge case testing:
  • viewport.test.js: thorough input validation (null, undefined, non-string types)
  • patterns.test.js: glob pattern matching edge cases
  • crawler.test.js: filesystem integration tests with proper temp directory cleanup
• Good use of real filesystem operations in tests (better than mocks for integration testing)

3. Puppeteer Optimizations

• CI-optimized browser args reduce memory footprint significantly
• protocolTimeout: 60_000 improves failure detection speed
• Navigation timeout fallback (networkidle2 → domcontentloaded) adds resilience

---

🐛 Potential Issues

1. Critical: Browser Args Applied Unconditionally (src/browser.js:60)

The CI_OPTIMIZED_ARGS are now applied to all browser launches, not just CI environments. This could negatively impact local development:

args: [...CI_OPTIMIZED_ARGS, ...args],

Issue: Flags like --disable-gpu, --js-flags=--max-old-space-size=512 may hurt performance on developer machines with GPUs and ample memory.

Recommendation:

const isCI = process.env.CI || process.env.GITHUB_ACTIONS || process.env.JENKINS_HOME;
const baseArgs = isCI ? CI_OPTIMIZED_ARGS : ['--no-sandbox', '--disable-setuid-sandbox'];
args: [...baseArgs, ...args],

2. Navigation Fallback Hides Real Issues (src/browser.js:102-116)

The try/catch silently falls back from networkidle2 to domcontentloaded:

catch (error) {
  if (error.message.includes('timeout') || error.message.includes('Navigation timeout')) {
    await page.goto(url, { waitUntil: 'domcontentloaded', ... });
  }
}

Issues:

• No logging/warning when fallback occurs
• domcontentloaded may result in screenshots of incomplete pages (async content still loading)
• Error string matching is fragile (locale-dependent error messages)

Recommendations:

• Add logging: console.warn('Navigation timeout, falling back to domcontentloaded')
• Consider making this behavior opt-in via config: { navigation: { fallback: true } }
• Check error type instead of message: error.name === 'TimeoutError'

3. Missing Input Validation (src/crawler.js:124-132)

Good addition of validation in generateStoryUrl, but the error handling could be more defensive:

if (\!baseUrl || typeof baseUrl \!== 'string') {
  throw new Error('baseUrl must be a non-empty string');
}

Issue: Doesn't validate against malformed URLs (e.g., baseUrl = 'not a url').

Recommendation:

try {
  new URL(baseUrl); // Validates URL format
} catch {
  throw new Error('baseUrl must be a valid URL');
}

---

🔍 Code Quality & Best Practices

4. Test Cleanup Could Be More Robust

Tests in crawler.test.js use tmpdir() + Date.now() for temp directories:

testDir = join(tmpdir(), `storybook-test-${Date.now()}`);
await rm(testDir, { recursive: true });

Minor Issue: If tests fail/crash, temp directories won't be cleaned up.

Recommendation: Use node:test hooks for guaranteed cleanup:

import { before, after } from 'node:test';

describe('readIndexJson', () => {
  let testDir;
  
  after(async () => {
    if (testDir) await rm(testDir, { recursive: true, force: true });
  });
  
  it('should read...', async () => {
    testDir = join(tmpdir(), `storybook-test-${Date.now()}`);
    // test code
  });
});

5. Hard-Coded Memory Limit May Be Too Restrictive

'--js-flags=--max-old-space-size=512', // Limit V8 heap

Concern: 512MB is quite low for modern JS apps. Large Storybooks may crash with OOM errors.

Recommendation:

• Make this configurable: --max-old-space-size=${process.env.VIZZLY_MAX_MEMORY || 512}
• Or increase default to 1024MB (still CI-friendly but less restrictive)

6. Test Script Uses Shell Command Substitution (package.json:44)

"test": "node --test --test-reporter=spec $(find tests -name '*.test.js')"

Issue: Won't work on Windows (no find command, and $(...) is bash-specific).

Recommendation:

"test": "node --test --test-reporter=spec tests/**/*.test.js"

Node's --test flag supports glob patterns natively (as of Node 22+, which is your minimum version).

---

🔒 Security

No security concerns identified. Input validation additions are a good security practice.

---

⚡ Performance

The Puppeteer optimizations should improve CI performance:

• Reduced protocol timeout (180s → 60s) fails faster
• Disabled unnecessary features reduce overhead
• Memory limits prevent resource exhaustion

However, concern #1 (unconditional CI args) may hurt local dev performance.

---

📝 Summary & Recommendations

This is a high-quality PR with excellent test coverage improvements. The migration from Vitest to Node.js test runner is clean and well-executed.

Must Fix:

1. Conditionally apply CI optimizations based on environment detection
2. Add logging/warnings for navigation fallback behavior

Should Fix:
3. Improve URL validation in generateStoryUrl
4. Fix Windows compatibility in test script
5. Use after() hooks for test cleanup

Consider:
6. Make memory limit configurable
7. Make navigation fallback opt-in via config

Overall: Approve with requested changes ✅

---

Test Plan Verification: ✅ All items in PR test plan are covered