🐛 Fix Vitest SDK status values and add SDK E2E tests to CI

[Robdel12]

Summary

• Fix Vitest SDK to use correct status values from TDD handler (match/diff instead of passed/failed)
• Add test-sdk-e2e CI job that always runs to catch CLI changes that break SDK integrations
• Remove redundant test-site job (consolidated into test-sdk-e2e)
• Add shared Playwright browser cache across SDK tests for faster CI

Fixes #179

Changes

Vitest SDK (clients/vitest/src/setup.js):

• Changed 'passed' → 'match' status check
• Changed 'failed' → 'diff' status check
• Added handling for 'baseline-updated' status

CI Workflow (.github/workflows/ci.yml):

• Added test-sdk-e2e job that runs on every PR/push (not conditional on SDK file changes)
• Tests Core JS Client, Vitest SDK, Ember SDK, and Ruby SDK end-to-end
• Consolidated test-site into test-sdk-e2e
• Shared Playwright browser cache (chromium + firefox) across all SDK tests

Context

PR #169 changed the TDD handler to return 'match'/'diff' instead of 'passed'/'failed', but the Vitest client wasn't updated. This caused the error "Unknown comparison status: match" reported in #179.

The SDK E2E tests would have caught this - they now run on every PR to prevent similar regressions.

See #180 for follow-up work to add Storybook and Static-site SDK E2E tests.

Test plan

• CLI lint passes
• CLI tests pass (1784/1784)
• Vitest client lint passes
• Vitest client unit tests pass (12/12)
• CI workflow YAML is valid
• CI test-sdk-e2e job passes (will verify in this PR)

[claude]

Code Review Summary

This PR correctly fixes the Vitest SDK status mismatch bug and adds valuable E2E testing infrastructure. The changes are well-structured and address the root cause effectively.

✅ Strengths

1. Correct Bug Fix

• The Vitest SDK now properly handles all status values returned by the TDD handler (src/server/handlers/tdd-handler.js:485-536):
  • 'match' for successful comparisons (was 'passed')
  • 'diff' for visual differences (was 'failed')
  • 'baseline-updated' for updated baselines (new handling)
  • 'new' for new baselines (unchanged)

2. Excellent CI Improvements

• New test-sdk-e2e job runs unconditionally, ensuring CLI changes don't break SDKs
• Smart consolidation: removed redundant test-site job
• Shared Playwright browser cache across SDKs (chromium + firefox) reduces CI time
• Tests multiple SDK integrations: Core JS, Vitest, Ember, and Ruby

3. Comprehensive Test Coverage

• E2E tests cover the full flow: test runner → SDK → CLI server → visual comparison
• Tests multiple integration points that unit tests can't catch

🔍 Observations & Suggestions

1. Test Coverage Gap (Low Priority)
The existing tests in tests/server/handlers/tdd-handler.test.js still use internal status values ('passed'/'failed') because they're testing the TDD service layer. However, there are no unit tests that verify the Vitest SDK's toMatchScreenshot function correctly handles the transformed status values.

The E2E tests will catch this, but consider adding unit/integration tests for clients/vitest/src/setup.js that mock the server response and verify each status branch:

• status: 'match' → { pass: true }
• status: 'diff' (above threshold) → { pass: false, message: 'Visual difference...' }
• status: 'diff' (within threshold) → { pass: true }
• status: 'baseline-updated' → { pass: true }
• status: 'new' → { pass: false, message: 'New screenshot...' }

This would catch regressions faster (unit tests run on every commit, E2E only in CI).

2. Threshold Logic Clarity
In clients/vitest/src/setup.js:108-118, the code checks comparison.diffPercentage <= threshold for diff status. This is correct, but there's a subtle question: does the TDD handler already return 'match' when diff is within threshold, or does it always return 'diff' when any visual difference exists?

Looking at the handler code (src/server/handlers/tdd-handler.js:485-499), it appears the TDD service returns 'failed' (mapped to 'diff') regardless of threshold, and the SDK must perform the threshold check. This is correct, but could be documented in a comment.

3. CI Workflow - Job Dependencies
The ci-check job now depends on test-sdk-e2e (line 615), which is correct. However, the job still checks conditional SDK test jobs that may skip. Verify that skipped jobs don't cause ci-check to fail (GitHub Actions treats skipped jobs as success, so this should be fine).

4. Error Handling Consistency
The setup.js already has excellent error handling (lines 132-153). No changes needed, just noting this is well done.

🔒 Security

No security concerns. The changes are confined to:

• Status value mappings (no user input processing)
• CI workflow (uses existing secrets properly)

⚡ Performance

Positive impact:

• Shared Playwright cache will significantly speed up CI
• Consolidating jobs reduces CI overhead

📝 Code Quality

Style & Conventions:

• Follows repository patterns ✓
• Clear comments explaining the SDK E2E job purpose ✓
• Consistent with other SDK integrations ✓

Potential Improvement:
The CI workflow has a lot of similar steps for each SDK. Consider creating a reusable composite action for "Run SDK E2E test" to reduce duplication, but this is a future enhancement (not blocking).

🧪 Test Plan Verification

From the PR description:

• CLI lint passes
• CLI tests pass (1784/1784)
• Vitest client lint passes
• Vitest client unit tests pass (12/12)
• CI workflow YAML is valid
• CI test-sdk-e2e job passes (will verify in this PR) ← This PR will test this

The test plan is thorough and appropriate.

📋 Recommendations

Must Fix: None - the PR is ready to merge as-is.

Should Consider (Non-blocking):

1. Add unit tests for the Vitest SDK status handling to catch regressions faster
2. Add a comment in setup.js:108 explaining why SDK performs threshold check (not server)

Nice to Have (Future):

1. Create reusable composite action for SDK E2E tests to reduce CI duplication
2. Add Storybook and Static-site SDK E2E tests (tracked in 🧪 Add E2E tests for Storybook and Static-site SDKs #180)

✨ Final Verdict

Approved! This PR:

• Correctly fixes the bug reported in Vitest: Unknown comparison status: match #179
• Adds important safety rails (E2E tests) to prevent similar regressions
• Improves CI efficiency with shared caching
• Follows repository conventions and best practices

The fix is minimal, correct, and well-tested by the E2E suite. Once the CI passes, this is ready to merge.

Great work connecting the dots between PR #169's backend changes and the client SDK impact! 🎯