-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Bump Vitest to v4 #19216
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: main
Are you sure you want to change the base?
Bump Vitest to v4 #19216
Conversation
Ideally we just repeat a step and create the shards dynamically, but not sure if that's even possible. With this approach, we will generate _a lot_ more integration tests though...
|
The Vitest tests timeout because of the repeated design system reloading + canonicalization cache prefilling. |
f11f8b0 to
8aa9f4c
Compare
WalkthroughThis pull request consolidates test infrastructure and optimization across the codebase. The integration test CI workflow is simplified from three sharded steps to one combined test per integration. The Vitest dependency is upgraded from ^2.0.5 to ^4.0.3. A new Pre-merge checks✅ Passed checks (3 passed)
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.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
.github/workflows/integration-tests.yml(1 hunks)integrations/upgrade/index.test.ts(1 hunks)integrations/utils.ts(2 hunks)package.json(1 hunks)packages/@tailwindcss-upgrade/src/codemods/css/migrate-at-apply.test.ts(2 hunks)packages/@tailwindcss-upgrade/src/codemods/template/is-safe-migration.test.ts(1 hunks)packages/tailwindcss/src/canonicalize-candidates.test.ts(26 hunks)vitest.config.ts(1 hunks)vitest.workspace.ts(0 hunks)
💤 Files with no reviewable changes (1)
- vitest.workspace.ts
🧰 Additional context used
🧬 Code graph analysis (3)
packages/@tailwindcss-upgrade/src/codemods/css/migrate-at-apply.test.ts (1)
packages/@tailwindcss-upgrade/src/codemods/css/migrate-at-apply.ts (1)
migrateAtApply(7-61)
packages/tailwindcss/src/canonicalize-candidates.test.ts (1)
integrations/utils.ts (1)
candidate(534-544)
packages/@tailwindcss-upgrade/src/codemods/template/is-safe-migration.test.ts (1)
packages/@tailwindcss-upgrade/src/stylesheet.ts (1)
designSystem(278-285)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: macOS
- GitHub Check: Windows
- GitHub Check: macOS / cli
- GitHub Check: macOS / oxide
- GitHub Check: macOS / vite
- GitHub Check: macOS / upgrade
- GitHub Check: Windows / cli
- GitHub Check: Windows / postcss
- GitHub Check: Windows / upgrade
- GitHub Check: Windows / vite
🔇 Additional comments (8)
packages/@tailwindcss-upgrade/src/codemods/css/migrate-at-apply.test.ts (2)
16-20: Approve perf workaround, but verify it doesn't affect test accuracy.The
@theme { --*: initial; }block is a reasonable short-term fix for the timeout issues mentioned in the PR objectives. However, ensure that resetting all theme tokens doesn't inadvertently mask bugs in theme-dependent migration logic.The TODO comment suggests this is temporary—consider opening an issue to track investigation of the root cause (canonicalization cache performance) so this workaround can eventually be removed.
93-133: LGTM—timeout and inline helper address known performance issues.The explicit 10-second timeout and inlined
migrateWithPrefixhelper are appropriate given the PR's timeout context. The perf theme block duplication (lines 105-108) is necessary because this test loads a different design system configuration (withprefix(tw)).Monitor that tests consistently complete well within the 10-second timeout. If tests still approach or exceed this limit, it may indicate that the perf workaround isn't sufficient or that deeper performance issues need investigation.
integrations/utils.ts (1)
40-40: LGTM! Clean implementation of per-test timeout support.The optional
timeoutfield and fallback pattern (config.timeout ?? TEST_TIMEOUT) allows individual tests to override the default timeout while maintaining backward compatibility. This is particularly useful for tests that inherently take longer to execute.Also applies to: 90-90
vitest.config.ts (1)
1-8: LGTM! Clear test discovery configuration.The configuration appropriately:
- Includes all package tests via glob pattern
- Excludes integration tests (run separately via
test:integrationsscript in the workflow)- Filters out
tsconfig.base.jsonto avoid treating it as a test projectThe separation of integration tests aligns with the updated CI workflow structure.
.github/workflows/integration-tests.yml (1)
106-107: Verify CI execution time impact of removing test sharding.The workflow simplifies from three sharded steps to one combined test per integration. While this improves clarity, it may increase total CI execution time if the previous sharding provided parallelization benefits within each integration.
Monitor the CI execution time for a few runs to ensure this change doesn't significantly increase build times. The matrix already provides parallelization across different integrations, so the impact may be minimal, but it's worth confirming.
package.json (1)
61-61: Vitest 4.0.3 is available and secure.Verification confirms that version 4.0.3 exists on npm registry. The CRITICAL RCE vulnerability published February 4, 2025, does not affect v4.x—it impacts only v1.x (< 1.6.1), v2.x (< 2.1.9), and v3.x (< 3.0.5). The major version upgrade is secure.
packages/@tailwindcss-upgrade/src/codemods/template/is-safe-migration.test.ts (2)
7-8: LGTM: Clean template literal helper.The
String.rawtag function is a standard pattern for avoiding escape sequences in template literals, which is appropriate for CSS strings.
10-22: ****The
--*: initial;pattern on line 16 is a documented Tailwind CSS v4 feature for completely disabling all default theme-driven utilities—not an undocumented pattern. The identical values for--shadowand--shadow-smon lines 17–18 do not affect test accuracy since the test cases only verify detection of theshadowtoken, notshadow-sm. The inline CSS approach is a sound performance optimization that aligns with the PR's goal of addressing test timeouts.No changes needed; the code is correct.
Likely an incorrect or invalid review comment.
This PR bumps Vitest from v2 to v4. As far as I know we don't use any Vitest specific features in our tests, but had to upgrade the
vitest.workspace.tsfile to avitest.config.tsfile instead.The only features we use are the typical
describe,it,test, andexpectfunctions.The only other part we use is
vi.spyOnandvi.fnbut those didn't change in API either.The test shards were removed to prevent errors. Not all suites have enough files / tests to be broken up into 3 parts so Vitest now errors when that happens.
Test plan
Enabling [ci-all], to make sure all tests pass on all platforms.