Skip to content

Conversation

@Robdel12
Copy link
Contributor

@Robdel12 Robdel12 commented Jan 17, 2026

Summary

This PR fixes SDK E2E tests to work properly with both vizzly tdd run (local TDD mode) and vizzly run (cloud mode), and reorganizes CI workflows for better maintainability.

Changes

CI Workflow Reorganization

  • Split monolithic ci.yml into focused workflow files:
    • sdk-e2e.yml - E2E tests for all SDKs (runs on every PR)
    • sdk-unit.yml - Unit tests with matrix testing (runs when SDK files change)
    • reporter.yml - Reporter UI tests
    • tui.yml - TUI tests
  • E2E tests now run in both TDD and cloud modes for each SDK

Ruby SDK Fixes

  • Add cloud_mode? helper to detect when running with VIZZLY_TOKEN
  • Add assert_screenshot_success helper for mode-aware assertions
  • TDD mode: asserts status is 'new' or 'match'
  • Cloud mode: asserts success is true (no local comparison happens)
  • Fixed in both integration_test.rb and e2e_test.rb

Vitest SDK Fixes

  • Refactored to use shared test-site for consistency with other SDKs
  • Test the actual SDK functionality (toMatchScreenshot matcher)
  • Load test-site CSS for consistent styling
  • Removed custom commands that were for full-page navigation, not SDK testing

All SDKs

  • Detect VIZZLY_SERVER_URL env var to skip starting own TDD server when running under vizzly tdd run or vizzly run
  • Fix assertions to handle both TDD mode (returns status field) and cloud mode (returns success: true)

Test plan

  • All SDK E2E tests pass in TDD mode (vizzly tdd run)
  • All SDK E2E tests pass in cloud mode (vizzly run)
  • Ruby lint passes (bundle exec rubocop)
  • CI workflow YAML is valid

@vizzly-testing
Copy link

🐻 Vizzly — 7 changes need review

Status Count
Passed 12
Changed 7
Auto-approved 12
Visual differences (7)

fullscreen-viewer · Firefox · 1920×1080 · 1.5% diff

fullscreen-viewer

search-no-results · Firefox · 1920×1080 · 0.1% diff

search-no-results

search-no-results · Firefox · 375×978 · 3.3% diff

search-no-results

bulk-accept-dialog · Firefox · 1920×1080 · 5.8% diff

bulk-accept-dialog

viewer-overlay-mode · Firefox · 1920×1080 · 1.6% diff

viewer-overlay-mode

viewer-zoomed-100 · Firefox · 1920×1080 · 0.3% diff

viewer-zoomed-100

...and 1 more in Vizzly.

Review changes →


fix-sdk-e2e-tests · 8369a5a3

@vizzly-testing
Copy link

🐻 Vizzly — 1 change needs review

Status Count
Changed 1
Auto-approved 4
Visual differences (5)

vizzly-help · 1202×1430 · 4.9% diff

vizzly-help

vizzly-invalid-command · 1202×578 · 12.0% diff

vizzly-invalid-command

vizzly-doctor · 1202×578 · 12.0% diff

vizzly-doctor

vizzly-run-help · 1202×722 · 9.6% diff

vizzly-run-help

vizzly-tdd-help · 1202×578 · 12.0% diff

vizzly-tdd-help

Review changes →


fix-sdk-e2e-tests · 8369a5a3

@claude
Copy link

claude bot commented Jan 17, 2026

Code Review - PR #183

This PR makes significant improvements to SDK E2E tests, ensuring they work correctly when run via vizzly tdd run. The changes fix API misuse, improve test structure, and add proper server detection.

Strengths

  1. Excellent Server Detection Pattern - The VIZZLY_SERVER_URL detection is clean and consistent
  2. Correct API Fixes - Fixed Vitest to use Testing Library, Static-Site to use URL paths, Ember assertions, Storybook patterns
  3. Comprehensive Test Coverage - All SDKs now test element/full screenshots, properties, thresholds, multiple pages
  4. Proper Config Structure - Fixed pageDiscovery nesting

Issues Found

CRITICAL

Command Injection in Ruby (clients/ruby/test/e2e_test.rb)
Uses string interpolation in shell command. While port is a number, this pattern is risky. Use spawn array syntax instead.

WARNINGS

  1. Hardcoded Timeouts - May fail on slow CI. Use env-based timeouts (CI ? 5000 : 2000)
  2. Missing Error Handling - Server spawn needs consistent error handling before promise resolves
  3. Double CI Execution - Tests run twice (TDD + Cloud). Consider making conditional to save CI time

SUGGESTIONS

  1. Test Site Coupling - All SDKs depend on ../../../test-site. Document this, add CI check
  2. Code Duplication - Extract shared E2E setup logic to test-utils
  3. Documentation - Add SDK README sections for E2E requirements and troubleshooting

Performance

  • CI time doubles with TDD+Cloud runs - monitor for bottlenecks
  • Browser instances could be reused within SDKs
  • Consider parallel test execution within each SDK

Verdict

Approve with minor fixes

Priority before merge:

  1. Fix Ruby command injection (low risk but best practice)
  2. Ensure consistent error handling in server spawns

Nice to have: shared E2E helpers, documentation, conditional CI execution

Great work improving test infrastructure!

@vizzly-testing
Copy link

🐻 Vizzly — 5 changes need review

Status Count
Passed 14
Changed 5
Auto-approved 14
Visual differences (5)

fullscreen-viewer · Firefox · 1920×1080 · 1.5% diff

fullscreen-viewer

viewer-overlay-mode · Firefox · 1920×1080 · 1.6% diff

viewer-overlay-mode

fullscreen-viewer · Firefox · 375×667 · 0.9% diff

fullscreen-viewer

search-no-results · Firefox · 375×978 · 3.5% diff

search-no-results

viewer-zoomed-100 · Firefox · 375×667 · 198.1% diff

viewer-zoomed-100

Review changes →


fix-sdk-e2e-tests · 31d6df25

@vizzly-testing
Copy link

🐻 Vizzly — 1 change needs review

Status Count
Changed 1
Auto-approved 4
Visual differences (5)

vizzly-help · 1202×1430 · 4.9% diff

vizzly-help

vizzly-doctor · 1202×578 · 12.0% diff

vizzly-doctor

vizzly-tdd-help · 1202×578 · 12.0% diff

vizzly-tdd-help

vizzly-invalid-command · 1202×578 · 12.0% diff

vizzly-invalid-command

vizzly-run-help · 1202×722 · 9.6% diff

vizzly-run-help

Review changes →


fix-sdk-e2e-tests · 31d6df25

@vizzly-testing
Copy link

vizzly-testing bot commented Jan 17, 2026

🐻 Vizzly — Approved ✓

19 comparisons approved.

Status Count
Passed 11
Changed 8
Auto-approved 11
Visual differences (8)

bulk-accept-dialog · Firefox · 1920×1080 · 5.1% diff

bulk-accept-dialog

viewer-overlay-mode · Firefox · 1920×1080 · 1.6% diff

viewer-overlay-mode

search-no-results · Firefox · 1920×1080 · 0.1% diff

search-no-results

fullscreen-viewer · Firefox · 1920×1080 · 1.5% diff

fullscreen-viewer

viewer-zoomed-100 · Firefox · 1920×1080 · 0.3% diff

viewer-zoomed-100

search-no-results · Firefox · 375×978 · 3.5% diff

search-no-results

...and 2 more in Vizzly.

View build →


fix-sdk-e2e-tests · 890d7460

@vizzly-testing
Copy link

vizzly-testing bot commented Jan 17, 2026

🐻 Vizzly — Approved ✓

5 comparisons approved.

Status Count
Changed 1
Auto-approved 4
Visual differences (5)

vizzly-help · 1202×1430 · 4.9% diff

vizzly-help

vizzly-tdd-help · 1202×578 · 12.0% diff

vizzly-tdd-help

vizzly-run-help · 1202×722 · 9.6% diff

vizzly-run-help

vizzly-doctor · 1202×578 · 12.0% diff

vizzly-doctor

vizzly-invalid-command · 1202×578 · 12.0% diff

vizzly-invalid-command

View build →


fix-sdk-e2e-tests · 890d7460

- Detect VIZZLY_SERVER_URL env var to skip starting own TDD server
- Fix Vitest tests to use correct browser mode API (getByRole, getByText)
- Fix Static-Site tests to use URL-style paths (/, /features) not filenames
- Fix Storybook pattern matching to use story.id format (*button*)
- Fix Ember assertions to check result.status instead of result.success
- Add proper config structure for page discovery (pageDiscovery.useSitemap)

All SDK E2E tests now pass when run via `vizzly tdd run`:
- Storybook: 13/13
- Static-Site: 13/13
- Ember: 9/9
- Vitest: 24/24
- Ruby: 10/10
@vizzly-testing

This comment was marked as outdated.

@vizzly-testing

This comment was marked as outdated.

- Add custom `loadPage` command to fetch and inject HTML content
- Start static server for test-site with dynamic port allocation
- Use real FluffyCloud test-site instead of inline HTML
- Screenshots now match other SDKs (Storybook, Static-Site, Ember, Ruby)

The `loadPage` command works around Vitest browser mode's limitation
of not supporting `page.goto()` by fetching HTML and using
`frame.setContent()` to inject it into the test iframe.
@vizzly-testing

This comment has been minimized.

@vizzly-testing

This comment has been minimized.

- Split monolithic ci.yml (900+ lines) into focused workflows:
  - ci.yml: Core CLI lint, tests, build (78 lines)
  - reporter.yml: Reporter visual tests (Vizzly cloud)
  - tui.yml: TUI visual tests (Vizzly cloud)
  - sdk-e2e.yml: All SDK E2E tests (TDD + Cloud mode)
  - sdk-unit.yml: SDK unit tests (conditional on file changes)

- Add cloud mode for Ruby, Storybook, and Static-Site SDKs
- Each workflow has its own status check job for branch protection
- SDK E2E always runs to catch CLI breaking changes
- SDK unit tests only run when that SDK's files change

New secrets needed:
- VIZZLY_RUBY_CLIENT_TOKEN
- VIZZLY_STORYBOOK_CLIENT_TOKEN
- VIZZLY_STATIC_SITE_CLIENT_TOKEN
@vizzly-testing

This comment has been minimized.

@vizzly-testing

This comment has been minimized.

@vizzly-testing

This comment has been minimized.

@vizzly-testing

This comment has been minimized.

- Test the actual SDK functionality (toMatchScreenshot matcher)
- Load test-site CSS for consistent styling with other SDKs
- Remove custom commands (loadPage was for full-page navigation, not SDK testing)
- Test page screenshots, element screenshots, properties, and thresholds
@vizzly-testing

This comment has been minimized.

@blacksmith-sh

This comment has been minimized.

@vizzly-testing

This comment has been minimized.

@vizzly-testing

This comment has been minimized.

@vizzly-testing

This comment has been minimized.

@vizzly-testing

This comment has been minimized.

@vizzly-testing

This comment has been minimized.

- Add cloud_mode? helper to detect when running with VIZZLY_TOKEN
- Add assert_screenshot_success helper for mode-aware assertions
- TDD mode: asserts status is 'new' or 'match'
- Cloud mode: asserts success is true (no local comparison)
@vizzly-testing

This comment has been minimized.

@vizzly-testing

This comment has been minimized.

Switch from blacksmith to ubuntu-latest for all CI workflows since this
is an open source project with free GitHub Actions minutes.

Keeps blacksmith only for release workflows (internal).
@vizzly-testing
Copy link

vizzly-testing bot commented Jan 19, 2026

🐻 Vizzly — Approved ✓

5 comparisons approved.

Status Count
Changed 1
Auto-approved 4

View build →


CLI TUI · fix-sdk-e2e-tests · 29e55456

@vizzly-testing
Copy link

vizzly-testing bot commented Jan 19, 2026

🐻 Vizzly — Approved ✓

19 comparisons approved.

Status Count
Passed 6
Changed 13
Auto-approved 6

View build →


CLI Reporter · fix-sdk-e2e-tests · 29e55456

@Robdel12 Robdel12 enabled auto-merge (squash) January 19, 2026 04:16
@Robdel12 Robdel12 merged commit c203bfa into main Jan 19, 2026
32 of 33 checks passed
@Robdel12 Robdel12 deleted the fix-sdk-e2e-tests branch January 19, 2026 04:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants