feat: Issue #34 - switching test setup to bun #35
Conversation
|
Warning Rate limit exceeded@ai16z-demirix has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 54 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (1)
WalkthroughThe codebase was migrated from the Vitest testing framework to Bun's native test runner. This involved updating test scripts, dependencies, mocking strategies, and type declarations. A new Bun test preload configuration and setup file were introduced to provide consistent mocks and environment preparation. Test files and documentation were updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner as Bun Test Runner
participant Preload as test-setup.ts
participant MockedCore as @elizaos/core (Mock)
participant Test as Test File
TestRunner->>Preload: Load preload setup (test-setup.ts)
Preload->>MockedCore: Mock core exports (logger, createUniqueUuid, etc.)
TestRunner->>Test: Execute test file
Test->>MockedCore: Import and use mocked core
Test->>TestRunner: Run assertions and report results
Poem
✨ 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
CodeRabbit Configuration File (
|
![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 (3)
src/services/__tests__/MessageService.test.ts (1)
1-18: Migration to Bun completed successfully.The imports and mocking setup are correctly updated for Bun. However, there's some redundancy with the preload mocking - the individual mock declarations and
mock.module()call might be unnecessary since@elizaos/coreis already mocked globally intest-setup.ts.Consider simplifying the mock setup since the global preload already handles
@elizaos/core:-// Mock the dependencies -const mockCreateUniqueUuid = mock((runtime, id) => `uuid-${id}`); -const mockLoggerError = mock(); -const mockLoggerDebug = mock(); - -mock.module("@elizaos/core", () => ({ - createUniqueUuid: mockCreateUniqueUuid, - logger: { - error: mockLoggerError, - debug: mockLoggerDebug, - }, -})); +// @elizaos/core is mocked in the test-setup.ts preload filesrc/__mocks__/@elizaos/core.ts (2)
17-20: Type the logger stubsGiving the stubs a signature (e.g.
(msg: string, ...meta: unknown[]) => void) avoidsanyleakage and catches incorrect usage inside tests.-const mockError = mock(); -const mockDebug = mock(); -const mockInfo = mock(); -const mockWarn = mock(); +const mockError = mock<(msg: string, ...meta: unknown[]) => void>(); +const mockDebug = mock<(msg: string, ...meta: unknown[]) => void>(); +const mockInfo = mock<(msg: string, ...meta: unknown[]) => void>(); +const mockWarn = mock<(msg: string, ...meta: unknown[]) => void>();
31-42: Freeze theEventTypeobject to preserve literal typesAppending
as constmakes every value a readonly string literal, giving you correct union types downstream while preventing accidental mutation.-export const EventType = { +export const EventType = { WORLD_JOINED: 'WORLD_JOINED', MESSAGE_CREATED: 'MESSAGE_CREATED', MESSAGE_RECEIVED: 'MESSAGE_RECEIVED', CHANNEL_CREATED: 'CHANNEL_CREATED', ENTITY_CREATED: 'ENTITY_CREATED', ENTITY_UPDATED: 'ENTITY_UPDATED', ENTITY_DELETED: 'ENTITY_DELETED', AGENT_CREATED: 'AGENT_CREATED', AGENT_UPDATED: 'AGENT_UPDATED', AGENT_DELETED: 'AGENT_DELETED', -}; +} as const; + +export type EventTypeKey = keyof typeof EventType;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
bunfig.toml(1 hunks)package.json(1 hunks)src/__mocks__/@elizaos/core.ts(1 hunks)src/__tests__/TESTING_GUIDE.md(4 hunks)src/__tests__/e2e/twitter-integration.test.ts(12 hunks)src/__tests__/environment.test.ts(13 hunks)src/client/__tests__/auth.test.ts(10 hunks)src/services/__tests__/MessageService.test.ts(13 hunks)src/services/__tests__/PostService.test.ts(14 hunks)src/test-setup.ts(1 hunks)tsconfig.json(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/__tests__/environment.test.ts (1)
src/environment.ts (2)
shouldTargetUser(63-83)validateTwitterConfig(103-220)
src/test-setup.ts (1)
src/__mocks__/@elizaos/core.ts (1)
UUID(5-5)
🔇 Additional comments (22)
bunfig.toml (1)
1-3: LGTM! Proper Bun test configuration.The
bunfig.tomlconfiguration correctly sets up test preloading for thesrc/test-setup.tsfile, which is essential for consistent mocking and environment setup across all tests in the Bun test runner.tsconfig.json (1)
13-13: LGTM! Proper TypeScript configuration for Bun.Adding
"bun-types"to the types array correctly enables TypeScript support for Bun's APIs and testing utilities, which is essential for the migration from Vitest to Bun's test framework.src/__tests__/environment.test.ts (3)
1-1: LGTM! Correct migration to Bun test framework.The import changes from Vitest to Bun's test framework are properly implemented.
14-14: LGTM! Proper API migration from Vitest to Bun.The changes correctly replace Vitest's
itwith Bun'stestfunction andvi.fn()withmock(). The test logic and assertions remain intact.Also applies to: 22-22, 27-27, 32-32, 41-41, 50-50, 61-61, 80-80, 88-88, 110-110, 131-131, 157-157, 176-176, 193-193, 208-208, 227-227, 239-239
158-160: No process.env mutations detected—removing stubEnv is safe
I searched all tests and found only reads ofprocess.env(no writes), so commenting outvi.stubEnvwon’t introduce cross-test interference. Removing the stub is safe.package.json (2)
27-27: LGTM! Proper dependency migration to Bun.The changes correctly remove Vitest dependencies and add
bun-typesfor TypeScript support with Bun's test framework.Also applies to: 30-30
36-40: LGTM! Well-structured test scripts for Bun.The new test scripts provide good granularity:
test:unitfor unit teststest:e2efor end-to-end teststest:allfor comprehensive testingtest:coveragefor coverage reportsThe script patterns correctly target the appropriate test directories.
src/client/__tests__/auth.test.ts (4)
1-1: LGTM! Correct import migration to Bun.The imports are properly changed from Vitest to Bun's test framework.
4-24: LGTM! Proper Bun module mocking setup.The module mocking approach correctly follows Bun's pattern:
- Create mock instances first
- Set up the mock module with
mock.module- Import the module after mocking
This ensures the mocks are properly established before the module is imported and used.
31-38: LGTM! Correct mock reset approach for Bun.The beforeEach correctly uses Bun's mock reset methods (
mockReset()andmockClear()) instead of Vitest's approach. The comment about "not needed with Bun's mocking approach" is appropriate.
48-48: LGTM! Proper API migration from Vitest to Bun.All test cases are correctly changed from
ittotestto match Bun's testing API. The test logic and assertions remain unchanged.Also applies to: 59-59, 66-66, 79-79, 86-86, 95-95, 145-145, 166-166, 194-194, 204-204, 227-227, 231-231
src/test-setup.ts (2)
1-6: LGTM! Good approach for centralized test setup.The preload file correctly uses Bun's mocking system and provides a centralized location for global mocks. The UUID type definition is consistent with the pattern used throughout the codebase.
8-37: Comprehensive mock implementation for @elizaos/core.The mock module covers all the essential exports needed by the test suites. The mock implementations are appropriate:
createUniqueUuidreturns predictable test values- Logger methods are properly mocked
- EventType and ChannelType enums include all necessary constants
src/services/__tests__/PostService.test.ts (3)
1-7: Successful migration to Bun test framework.The imports are correctly updated to use
bun:testinstead of Vitest, and the comment appropriately notes that@elizaos/coreis now mocked in the preload file.
16-16: Consistent UUID type casting throughout tests.The UUID type casting using template literals is applied consistently across all test cases. The change to individual field assertions instead of deep equality is a good improvement for handling dynamic fields like timestamps.
Also applies to: 54-55, 68-84
111-130: Proper spy management in logger test.Good practice using
spyOnwithmockRestore()to ensure test isolation and proper cleanup of mocks.src/services/__tests__/MessageService.test.ts (1)
28-28: Consistent UUID type casting applied.The UUID template literal type casting is consistently applied across all test cases, maintaining type safety while adapting to the new testing framework.
Also applies to: 76-76, 91-92
src/__tests__/TESTING_GUIDE.md (2)
44-54: Comprehensive update of test commands.All test commands have been correctly updated from
npm testtobun test, including pattern matching and watch mode examples. The syntax is appropriate for Bun's CLI.Also applies to: 62-66
202-248: Excellent documentation of the new Bun test setup.The new section provides clear, comprehensive guidance on:
- Bun's test runner configuration
- Mocking strategy with
mock()andmock.module()- TypeScript compatibility with UUID template literals
- Test organization and patterns
The examples are consistent with the actual implementation in the codebase.
src/__tests__/e2e/twitter-integration.test.ts (3)
1-30: Well-structured migration with local type definitions.The migration to
bun:testis clean, and the localIAgentRuntimetype definition is a smart approach to avoid external dependencies in E2E tests. This makes the tests more self-contained and less prone to import issues.
33-37: Good TypeScript compatibility solution.The
UuidStringtype andasUuidhelper function provide a clean way to handle UUID template literal types while maintaining compatibility. This pattern is consistent with the approach documented in the testing guide.
61-72: Consistent UUID casting in runtime and cleanup logic.The UUID type casting is properly applied to the agent ID and throughout the cleanup logic, ensuring type consistency across all test operations.
Also applies to: 96-96
|
@ChristopherTrimboli @wtfsayo please review when available |
|
|
|
@wtfsayo done |
|
Any more changes/fixes required here? @wtfsayo |
|
remove local-ai dependency + bun lock checked in |
On it sir! 🫡 |
|
Done @wtfsayo |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Bug: Test Fails Due to Missing Environment Variable
The test "should prioritize config over runtime over env" cannot fully verify the config > runtime > env priority chain. The vi.stubEnv call for TWITTER_API_KEY is commented out, with a note that Bun uses process.env directly, but no alternative is provided to set the environment variable for the test.
src/__tests__/environment.test.ts#L156-L161
plugin-twitter/src/__tests__/environment.test.ts
Lines 156 to 161 in 948e73c
Was this report helpful? Give feedback by reacting with 👍 or 👎
2 participants
Issue related: #34
Summary by CodeRabbit
Chores
Documentation
Tests