-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
feat: tests for supabase and sqlite db adapters #2468
Conversation
📝 WalkthroughWalkthroughThe pull request introduces comprehensive testing infrastructure for SQLite and Supabase database adapters using Vitest. It adds test suites for Changes
Possibly related issues
Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
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: 0
🧹 Nitpick comments (7)
packages/adapter-sqlite/__tests__/sqlite-adapter.test.ts (1)
8-16
: Consider using a dedicated mock file for elizaLogger.The inline mock could be moved to a separate mock file to improve maintainability.
packages/adapter-supabase/package.json (1)
26-29
: Lock the Vitest dependency versions.Using caret ranges (
^
) for test dependencies can lead to inconsistent test behavior. Consider using fixed versions.- "@vitest/coverage-v8": "^3.0.2", - "@vitest/ui": "^0.34.7", - "vitest": "^3.0.2" + "@vitest/coverage-v8": "3.0.2", + "@vitest/ui": "0.34.7", + "vitest": "3.0.2"packages/adapter-sqlite/package.json (1)
29-31
: Lock the Vitest dependency versions.Similar to the Supabase adapter, use fixed versions for test dependencies.
- "vitest": "^3.0.2", - "@vitest/coverage-v8": "^3.0.2", - "@vitest/ui": "^3.0.2" + "vitest": "3.0.2", + "@vitest/coverage-v8": "3.0.2", + "@vitest/ui": "0.34.7"packages/adapter-supabase/__tests__/supabase-adapter.test.ts (4)
10-11
: Consider using a more type-safe approach instead of type assertion.Replace
...actual as any
with proper type definitions to maintain type safety.- ...actual as any, + ...(actual as typeof import('@elizaos/core')),
46-47
: Avoid @ts-ignore by properly typing the mock.Instead of using @ts-ignore, consider defining proper types for the mock implementation.
- // @ts-ignore - we're mocking the implementation - adapter.supabase = mockSupabase; + (adapter as any).supabase = mockSupabase; // TODO: Consider exposing a protected property for testing
103-141
: Add test for empty participants array.Consider adding a test case where the query returns an empty array to ensure proper handling of users with no participants.
it('should return empty array when user has no participants', async () => { mockSupabase.eq.mockResolvedValueOnce({ data: [], error: null }); const userId = 'user-no-participants' as UUID; const result = await adapter.getParticipantsForAccount(userId); expect(result).toEqual([]); });
223-225
: Enhance error message with more details.The current error message could be more informative by including the error details.
- .toThrow('Failed to set participant user state'); + .toThrow(`Failed to set participant user state: ${error.message}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/adapter-sqlite/__tests__/sqlite-adapter.test.ts
(1 hunks)packages/adapter-sqlite/package.json
(1 hunks)packages/adapter-sqlite/vitest.config.ts
(1 hunks)packages/adapter-supabase/__tests__/supabase-adapter.test.ts
(1 hunks)packages/adapter-supabase/package.json
(1 hunks)packages/adapter-supabase/vitest.config.ts
(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/adapter-supabase/vitest.config.ts
- packages/adapter-sqlite/vitest.config.ts
🔇 Additional comments (4)
packages/adapter-sqlite/__tests__/sqlite-adapter.test.ts (2)
72-99
: Add edge case tests for getParticipantsForAccount.The current tests cover basic scenarios but miss important edge cases:
- Invalid user ID
- Database errors
159-170
: Add error handling tests for init and close methods.The current tests only verify successful scenarios. Add tests for:
- Database initialization failures
- Connection closure errors
packages/adapter-supabase/__tests__/supabase-adapter.test.ts (2)
56-101
: Well-structured test suite for getRoom!Good coverage of success, failure, and error cases with proper assertions.
143-191
: Excellent test coverage for getParticipantUserState!Good handling of mock chains and verification of multiple eq calls.
Relates to
#2467
Risks
Low: adding tests
Background
What does this PR do?
#2467
What kind of change is this?
features/tests
Documentation changes needed?
None
Testing
Where should a reviewer start?
packages/adapter-sqlite
packages/adapter-supabase
Detailed testing steps
pnpm install in root
navigate to desired directories to see tests
run pnpm test to run tests