feat: replace jest with vitest - #159
Conversation
📝 WalkthroughWalkthroughThe JavaScript testing framework for the project was migrated from Jest to Vitest. The test script in Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
package.json (1)
38-51: Clean up unused Jest artifactsSince you've migrated to Vitest, consider removing the unused Jest configuration and related dependencies to keep the package.json clean.
"devDependencies": { - "@types/jest": "29.5.11", "@types/mocha": "10.0.6", "@types/node": "22.5.1", "husky": "9.1.7", "lint-staged": "15.4.3", "prettier": "3.4.2", "ts-node": "10.9.2", "typescript": "5.7.2", "vitest": "3.2.4" }, - "jest": { - "testTimeout": 200000 - },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
package.json(2 hunks)src/ts/test/nft.test.ts(1 hunks)src/ts/test/token.test.ts(2 hunks)src/ts/test/utils.ts(2 hunks)vitest.config.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/ts/test/utils.ts (2)
benchmarks/token_contract.benchmark.ts (1)
setup(33-41)benchmarks/nft_contract.benchmark.ts (1)
setup(24-32)
🔇 Additional comments (8)
package.json (1)
18-18: LGTM: Clean migration to VitestThe test script update from Jest to Vitest is correctly implemented.
vitest.config.ts (1)
1-10: LGTM: Well-configured for Aztec sandbox testsThe Vitest configuration appropriately addresses the needs of long-running Aztec sandbox tests with extended timeouts and disabled file parallelism. The timeout values match the original Jest configuration.
src/ts/test/nft.test.ts (1)
18-18: LGTM: Correct Vitest import additionThe explicit import of Vitest testing functions is necessary for the migration and correctly includes all required functions.
src/ts/test/token.test.ts (2)
16-16: LGTM: Correct Vitest import additionThe explicit import of Vitest testing functions is correctly implemented.
452-452: Confirm Intentional Skip of the ‘Token – Multi PXE’ Test SuiteI didn’t find any TODOs or issue references associated with this skip. Please verify:
- File:
src/ts/test/token.test.ts(around line 452)// currently skipped – please confirm this is intentional describe.skip('Token - Multi PXE', () => {- If these tests fail under the Vitest migration or PXE support is incomplete, add a comment or link to the issue explaining the skip.
- Otherwise, re-enable the suite (change to
describe('Token - Multi PXE', …)) so any hidden failures surface.src/ts/test/utils.ts (3)
18-18: LGTM: Necessary Vitest importThe explicit import of
expectfrom Vitest is required since this utility file uses expect in helper functions.
26-30: LGTM: Improved configuration object constructionThe simplified object literal construction improves readability while maintaining the same functionality.
39-39: LGTM: Useful logging additionThe console.log statement provides helpful feedback during test setup, which can aid in debugging test issues.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/tests.yaml (2)
67-69:script -e -cwrapper seems unnecessary and adds indirectionThe GitHub runner already fails the step if the wrapped command exits non-zero, so wrapping
aztec-nargo compileinscript -e -cprovides little value while slightly slowing the job and obscuring logs (they are written through a pseudo-TTY).- - name: Compile - run: script -e -c "aztec-nargo compile" + - name: Compile + run: aztec-nargo compileUnless you specifically need the TTY recording produced by
script, dropping the wrapper simplifies maintenance and troubleshooting.
82-82: Add a trailing newline to satisfy lintersYAMLlint flags the missing EOF newline. Append one to avoid continual linter noise.
- run: script -e -c "yarn test:js" + run: script -e -c "yarn test:js" +
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/tests.yaml(2 hunks)src/ts/test/token.test.ts(3 hunks)src/ts/test/utils.ts(1 hunks)vitest.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/ts/test/utils.ts
- vitest.config.ts
- src/ts/test/token.test.ts
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/tests.yaml
[error] 82-82: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (1)
.github/workflows/tests.yaml (1)
81-82: Verify Vitest exit codes and reporting are surfaced in CISwitching to
yarn test:jsis fine, but make sure the underlying script:
- Runs Vitest in CI mode (
vitest run) rather than watch mode.- Exits with a non-zero status when tests fail (default for
vitest run, but double-check yourpackage.json).- Generates coverage or junit reports if those were previously consumed by the pipeline.
If any of those behaviours were handled by the former Jest invocation flags, update the
test:jsscript accordingly so the workflow keeps the same guarantees.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/tests.yaml (1)
81-82: Add a trailing newline to satisfy YAML-lintThe linter reports a missing end-of-file newline. Appending one keeps the file POSIX-compliant and silences the warning.
run: script -e -c "yarn test:js" +
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/tests.yaml(2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/tests.yaml
[error] 82-82: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: setup-and-run (ubuntu-latest-m, 12)
- GitHub Check: benchmark (ubuntu-latest-m, 12)
🔇 Additional comments (1)
.github/workflows/tests.yaml (1)
68-68: Compilation step looks goodSwitching to a direct
aztec-nargo compilecall keeps the workflow clear and removes the now-unused$AZTEC_NARGOindirection. Thescript -ewrapper properly forwards the exit code, so failures will bubble up.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/ts/test/utils.ts (1)
42-52: Good refactoring, but consider optimizing node client creation.The function correctly uses the new lazy initialization pattern. However, there's a minor inefficiency where two node clients are created for the same NODE_URL (one in
initializeConfig, one here).Consider this optimization to reuse the node client:
+let nodeClient: ReturnType<typeof createAztecNodeClient>; const initializeConfig = async () => { if (!l1Contracts) { - const node = createAztecNodeClient(NODE_URL); + nodeClient = nodeClient || createAztecNodeClient(NODE_URL); - l1Contracts = await node.getL1ContractAddresses(); + l1Contracts = await nodeClient.getL1ContractAddresses(); // ... rest unchanged } return fullConfig; }; export const setupPXE = async () => { const config = await initializeConfig(); - const node = createAztecNodeClient(NODE_URL); + const node = nodeClient || createAztecNodeClient(NODE_URL); // ... rest unchanged };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/ts/test/utils.ts(1 hunks)
🔇 Additional comments (2)
src/ts/test/utils.ts (2)
13-13: LGTM! Clean import updates for Vitest migration.The type imports for
PXEServiceConfigandL1ContractAddressesimprove type safety, and theexpectimport from Vitest correctly replaces Jest's expect function.Also applies to: 16-16, 19-19
56-109: LGTM! Well-structured utility functions.The utility functions are well-implemented with good separation of concerns and proper use of the Vitest expect function. The token and NFT deployment helpers follow good patterns and will be useful for testing.
| let l1Contracts: L1ContractAddresses; | ||
| let fullConfig: PXEServiceConfig & { l1Contracts: L1ContractAddresses }; | ||
|
|
||
| const initializeConfig = async () => { | ||
| if (!l1Contracts) { | ||
| const node = createAztecNodeClient(NODE_URL); | ||
| l1Contracts = await node.getL1ContractAddresses(); | ||
| const config = getPXEServiceConfig(); | ||
| fullConfig = { | ||
| ...config, | ||
| l1Contracts, | ||
| proverEnabled: false, | ||
| }; | ||
| } | ||
| return fullConfig; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider error handling and test isolation for the lazy initialization pattern.
The lazy initialization pattern is a good improvement over top-level async calls. However, consider these enhancements:
- Add error handling for the network call:
const initializeConfig = async () => {
if (!l1Contracts) {
- const node = createAztecNodeClient(NODE_URL);
- l1Contracts = await node.getL1ContractAddresses();
+ try {
+ const node = createAztecNodeClient(NODE_URL);
+ l1Contracts = await node.getL1ContractAddresses();
+ } catch (error) {
+ logger.error('Failed to initialize L1 contract addresses:', error);
+ throw error;
+ }
// ... rest of the function
}
return fullConfig;
};- Consider test isolation: The global caching might cause issues if tests need different configurations. You may want to add a reset function for test cleanup.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let l1Contracts: L1ContractAddresses; | |
| let fullConfig: PXEServiceConfig & { l1Contracts: L1ContractAddresses }; | |
| const initializeConfig = async () => { | |
| if (!l1Contracts) { | |
| const node = createAztecNodeClient(NODE_URL); | |
| l1Contracts = await node.getL1ContractAddresses(); | |
| const config = getPXEServiceConfig(); | |
| fullConfig = { | |
| ...config, | |
| l1Contracts, | |
| proverEnabled: false, | |
| }; | |
| } | |
| return fullConfig; | |
| }; | |
| let l1Contracts: L1ContractAddresses; | |
| let fullConfig: PXEServiceConfig & { l1Contracts: L1ContractAddresses }; | |
| const initializeConfig = async () => { | |
| if (!l1Contracts) { | |
| try { | |
| const node = createAztecNodeClient(NODE_URL); | |
| l1Contracts = await node.getL1ContractAddresses(); | |
| } catch (error) { | |
| logger.error('Failed to initialize L1 contract addresses:', error); | |
| throw error; | |
| } | |
| const config = getPXEServiceConfig(); | |
| fullConfig = { | |
| ...config, | |
| l1Contracts, | |
| proverEnabled: false, | |
| }; | |
| } | |
| return fullConfig; | |
| }; |
🤖 Prompt for AI Agents
In src/ts/test/utils.ts around lines 25 to 40, the lazy initialization function
initializeConfig lacks error handling for the asynchronous network call to
getL1ContractAddresses. Add a try-catch block around the async call to catch and
handle potential errors gracefully. Additionally, to improve test isolation,
implement a reset function that clears the cached l1Contracts and fullConfig
variables, allowing tests to start with a clean state and avoid interference
from previous test runs.
Benchmark ComparisonContract: nft
Contract: token
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||
|
|
||
| describe('Token - Multi PXE', () => { | ||
| describe.skip('Token - Multi PXE', () => { |
There was a problem hiding this comment.
TBF I would remove it. We only test a single flow (private transfer), I'd re add it once they're done changing the PXE's Notes interface oooor push to make them as extensive as the single PXE tests.
I commented it because I wasn't able to start the secondary PXE, I'll try again later.
2 participants
🤖 Linear
Closes AZT-XXX
Description
Summary by CodeRabbit
Chores
Tests