-
Notifications
You must be signed in to change notification settings - Fork 2.2k
RFC: Migrate from tape to vitest #9966
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Proposes replacing tape (assertions) + ocular-test (runner) with vitest. See dev-docs/RFCs/proposals/vitest-migration-rfc.md for full details. Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add vitest workspace configuration (node/browser/headless projects) - Clarify entry points: browser as source of truth, node as smoke test - Add Playwright rationale (required by vitest browser mode) - Expand Phase 4 with discovery mechanism for browser-only tests - Add detailed Phase 5 for snapshot/interaction test migration - Add Verification section with test commands - Update risks table with new considerations Co-Authored-By: Claude <[email protected]>
- Add 3-phase deprecation plan (9.3.x → 9.4.x → 10.0.0) - Include migration guide for external consumers - Resolve open question #3 about tape deprecation timeline Co-Authored-By: Claude <[email protected]>
felixpalmer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm in favour of this migration, not having an easy way to do TDD has long been a pain point.
My worry is that there will be a long tail of specific patterns that we have developed over the years to work with tape which will take time to port to vitest. On the other hand, this sort of migration seems the perfect task for an AI agent to tackle, so it's certainly worth trying.
|
|
||
| ## Open Questions | ||
|
|
||
| 1. Should we convert test file structure to use `describe`/`it` blocks, or keep flat `test()` calls? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will already be a huge PR, I would keep the test() calls for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. My approach is to vibe code a code mod and that would keep the structure the same, just convert syntax.
| ## Verification | ||
|
|
||
| 1. `yarn test-node` - runs unit tests in Node (fast feedback) | ||
| 2. `yarn test-browser` - runs ALL tests in Chromium (unit + render + interaction) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When testing I use yarn test browser as I can be more confident that it'll match what CI does. My biggest irritation is how the render tests open a browser window and steals focus. My preference for a fast feedback loop would be to run in the browser, but skipping render tests. Generally I disable render tests for initial testing and then run them when I'm mostly done with a feature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's annoying. Let's have the goal be to make the primary command headless and match between local and CI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proposal: going forward "yarn test-browser" will always open a browser with a vitest runner UI, and perform all tests including render tests.
To just run the test suite in a headless browser without any browser opening and stealing focus, it'll be yarn test-headless
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 5 potential issues.
|
|
||
| **1.1 Install dependencies:** | ||
| ```bash | ||
| yarn add -D @vitest/browser @vitest/browser-playwright playwright |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing core vitest package in installation dependencies
High Severity
Phase 1 installation command only adds browser-specific packages (@vitest/browser, @vitest/browser-playwright, playwright) but omits the core vitest package itself. Since this RFC proposes migrating FROM tape TO vitest, the core vitest package doesn't exist yet and must be included in the installation. Without it, none of the vitest commands in the RFC will work.
Ha, exactly! After a few false starts I pointed Conductor and Claude Code to make a idempotent conversion script that fed |
| **Decision point after discovery:** | ||
| - **Few failures (~10-20%)** → Keep hybrid, rename failures to `.browser.spec.ts` | ||
| - **Many failures (~50%+)** → Fall back to browser-only approach |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to run all of the tests in the node environment resulted in a lot of failures (which is not a surprise), so I'm going to instead implement the same test config for node that we have on master - use node for a smoke-test, and browser for all tests.
| |---------|-------------| | ||
| | `*.spec.ts` | Default - runs in both environments | | ||
| | `*.browser.spec.ts` | Browser-only (WebGL, real DOM, etc.) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are so few node tests that I'm actually going to flip the pattern. Default runs in browser, *.node.spec.ts runs in both.
| ## Verification | ||
|
|
||
| 1. `yarn test-node` - runs unit tests in Node (fast feedback) | ||
| 2. `yarn test-browser` - runs ALL tests in Chromium (unit + render + interaction) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proposal: going forward "yarn test-browser" will always open a browser with a vitest runner UI, and perform all tests including render tests.
To just run the test suite in a headless browser without any browser opening and stealing focus, it'll be yarn test-headless
- Flip file naming: *.node.spec.ts (node) vs *.spec.ts (browser) - Change to vitest.workspace.ts with defineWorkspace - Add CLI command mapping and matrix tables - Document Phase 4 outcome: ~95% tests need browser - List node smoke tests (2 files) and excluded tests Co-Authored-By: Claude <[email protected]>
Background
This RFC proposes modernizing deck.gl's test infrastructure by replacing tape (assertion framework) + ocular-test (runner) with vitest. The migration would consolidate tooling, improve developer experience, and maintain CLI compatibility.
I'd like to learn about features/workflows maintainers used that I can test against vitest. I primarily used CI tests, which includes node, browser, and render tests.
Change List
dev-docs/RFCs/proposals/vitest-migration-rfc.mdNote
Low Risk
Documentation-only change (new RFC) with no runtime or build behavior changes; risk is limited to potential future implementation decisions guided by the proposal.
Overview
Adds a draft RFC (
dev-docs/RFCs/proposals/vitest-migration-rfc.md) proposing a phased migration of deck.gl’s test infrastructure fromtape+ocular-testtovitest, including a multi-environment Vitest workspace setup (node smoke tests + Playwright-driven browser runs).The document defines CLI/script mappings to preserve existing workflows, outlines required test API/assertion conversions and
@deck.gl/test-utilschanges (e.g.,makeSpy→vi.spyOn), and calls out snapshot/interaction test migration considerations (Puppeteer → Playwright) plus risks/mitigations and a deprecation timeline.Written by Cursor Bugbot for commit e7c63c4. This will update automatically on new commits. Configure here.