test(build): migrate error tests to vitest#7043
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a new Vitest test suite at packages/build/tests/error/error.test.ts covering many build-error scenarios, tracing-attribute mappings, and trusted-plugin severity checks. Removes legacy AVA test and its snapshots (packages/build/tests/error/tests.js and snapshots). Changes testing fixtures: Fixture constructor now requires an explicit test file path; a new AvaFixture subclass supplies AVA-specific behavior; testing package exports updated to expose fixture and normalize modules. Refactors cleanUserEnv in packages/config to filter READONLY_ENV directly and removes the unused omit.js dependency. No public API types were changed. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/build/tests/error/error.test.ts (1)
263-268: 💤 Low valueConsider using
test.eachfor parameterized tests.Vitest provides
test.eachfor data-driven tests, which offers better IDE integration and clearer test reporting.♻️ Optional refactor using test.each
-testMatrixAttributeTracing.forEach(({ description, input, expects }) => { - test(`Tracing attributes - ${description}`, async () => { - const attributes = buildErrorToTracingAttributes(input) - expect(attributes).toEqual(expects) - }) -}) +test.each(testMatrixAttributeTracing)( + 'Tracing attributes - $description', + ({ input, expects }) => { + const attributes = buildErrorToTracingAttributes(input) + expect(attributes).toEqual(expects) + } +)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/build/tests/error/error.test.ts` around lines 263 - 268, The test uses a forEach loop over testMatrixAttributeTracing to generate parameterized tests; replace it with Vitest's test.each to improve IDE integration and test reporting. Update the block that iterates testMatrixAttributeTracing so it calls test.each(testMatrixAttributeTracing) with a callback using the tuple/object shape (description, input, expects) and inside assert buildErrorToTracingAttributes(input) toEqual expects; keep the same symbols testMatrixAttributeTracing and buildErrorToTracingAttributes so locating and adapting the existing tests is straightforward.packages/testing/package.json (1)
5-9: 💤 Low valueSubpath exports enable Vitest usage, but TypeScript consumers may lack type hints.
The exports map correctly enables direct imports like
@netlify/testing/lib/fixture.js. However, TypeScript consumers using these subpath exports won't automatically resolve type declarations since there's no correspondingtypesfield for each subpath.If TypeScript support for subpath imports is needed:
♻️ Optional: Add types for subpath exports
"exports": { ".": "./lib/index.js", - "./lib/fixture.js": "./lib/fixture.js", - "./lib/normalize.js": "./lib/normalize.js" + "./lib/fixture.js": { + "types": "./lib/fixture.d.ts", + "default": "./lib/fixture.js" + }, + "./lib/normalize.js": { + "types": "./lib/normalize.d.ts", + "default": "./lib/normalize.js" + } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/testing/package.json` around lines 5 - 9, The exports map exposes subpaths like "./lib/fixture.js" and "./lib/normalize.js" but doesn’t provide corresponding TypeScript declaration entries, so add type mappings for each exported subpath (or a top-level "types" entry plus "typesVersions" if you need path-specific resolution). Update package.json to include either per-subpath "types" fields or a top-level "types" pointing to your bundled .d.ts (and/or add a "typesVersions" map) so TypeScript consumers importing "@netlify/testing/lib/fixture.js" will resolve declarations for Fixture/normalize/index exports.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/build/tests/error/error.test.ts`:
- Around line 103-112: The TODO comment referencing "Ava v4" is stale after
migrating tests to Vitest; update the comment around the skipped "Early exit"
test (the block checking platform !== 'win32' and the test name 'Early exit'
that uses new Fixture(...).runWithBuild()) to reference Vitest instead of Ava,
re-evaluate whether the Windows-specific flakiness still occurs under Vitest,
and either adjust the skip logic or remove the outdated TODO so the comment
accurately describes the current test framework and next steps.
---
Nitpick comments:
In `@packages/build/tests/error/error.test.ts`:
- Around line 263-268: The test uses a forEach loop over
testMatrixAttributeTracing to generate parameterized tests; replace it with
Vitest's test.each to improve IDE integration and test reporting. Update the
block that iterates testMatrixAttributeTracing so it calls
test.each(testMatrixAttributeTracing) with a callback using the tuple/object
shape (description, input, expects) and inside assert
buildErrorToTracingAttributes(input) toEqual expects; keep the same symbols
testMatrixAttributeTracing and buildErrorToTracingAttributes so locating and
adapting the existing tests is straightforward.
In `@packages/testing/package.json`:
- Around line 5-9: The exports map exposes subpaths like "./lib/fixture.js" and
"./lib/normalize.js" but doesn’t provide corresponding TypeScript declaration
entries, so add type mappings for each exported subpath (or a top-level "types"
entry plus "typesVersions" if you need path-specific resolution). Update
package.json to include either per-subpath "types" fields or a top-level "types"
pointing to your bundled .d.ts (and/or add a "typesVersions" map) so TypeScript
consumers importing "@netlify/testing/lib/fixture.js" will resolve declarations
for Fixture/normalize/index exports.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6288e81a-ebc8-4ad3-a5f4-80cd15e27c75
⛔ Files ignored due to path filters (2)
packages/build/tests/error/__snapshots__/error.test.ts.snapis excluded by!**/*.snappackages/build/tests/error/snapshots/tests.js.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
packages/build/tests/error/error.test.tspackages/build/tests/error/snapshots/tests.js.mdpackages/build/tests/error/tests.jspackages/config/src/env/main.tspackages/testing/package.jsonpackages/testing/src/fixture-ava.tspackages/testing/src/fixture.tspackages/testing/src/index.ts
💤 Files with no reviewable changes (2)
- packages/build/tests/error/snapshots/tests.js.md
- packages/build/tests/error/tests.js
Migrates the error tests to vitest and alters the `testing` package such that `Fixture` is the ava-flavoured class (i.e. it imports `ava`). Temporarily, the package exports now declare `fixture.js` so non-ava consumers can import the base `Fixture` without importing `ava`. The `omit.js` dependency causes CJS interop problems in vitest, so has been dropped and replaced with a basic `Object.fromEntries` map.
43081j
force-pushed
the
avagone-error-tests
branch
from
41c3591 to
f6bf61a
Compare
1 participant
Migrates the error tests to vitest and alters the
testingpackage such thatFixtureis the ava-flavoured class (i.e. it importsava). Temporarily, the package exports now declarefixture.jsso non-ava consumers can import the baseFixturewithout importingava.The
omit.jsdependency causes CJS interop problems in vitest, so has been dropped and replaced with a basicObject.fromEntriesmap.NOTE: If you'd rather we just go all-in and update
Fixturedirectly, I can also do that. It means the diff will be pretty big because i'll have to update eachFixture(p)toFixture(testFile, p)basically.FYI i compared the snapshots using this script i threw together:
https://gist.github.com/43081j/e085304049ee734d7531dcb1855fa53f
they do match