fix: Supabase Mock Configuration in Tests #206
Conversation
📝 WalkthroughWalkthroughThe PR resolves failing Supabase mock configuration in tests by introducing a centralized, Bun-compatible mock implementation. It replaces scattered jest-style spies with Bun's native mock module system, updates the Soroban RPC import path, and refactors test setup to apply mocks before module imports. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
|
Let me know if any changes are required, happy to help! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/backend/tests/integration/booking.test.ts (1)
75-97: Mock may not take effect for pre-built app.The
appis built inbeforeAll()with the originalgetBookingcontroller. Callingmock.module()in individual tests won't affect the already-imported controller that's bound to the Express app.Notice how the 200 OK test (lines 223-228) correctly handles this by re-importing the controller after setting up the mock and creating a new Express app. The same pattern should be applied to these error scenario tests, or the app should be rebuilt after each mock setup.
💡 Suggested approach
Either:
- Apply the same pattern as the 200 OK test (re-import and rebuild app after mock)
- Or move mock setup to
beforeAlland use per-test mock return values
🤖 Fix all issues with AI agents
In `@apps/backend/src/tests/sync.service.test.ts`:
- Around line 145-158: The test names claim event processing and error handling
but only start/stop the service (syncService.start(), syncService.getStatus(),
syncService.stop()); either rename the two it(...) descriptions to something
like "should start and stop service during event processing" and "should start
and stop service when event processing errors occur", or expand the tests to
assert real behavior: after syncService.start() emit or call the actual event
handler (e.g., syncService.processEvent or the event emitter used by the
service) with a mock "booking created" payload and assert the expected handler
was invoked and side-effects occurred, and for the error test stub the handler
to throw/reject and assert the service stays alive, logs the error, and
recovers/continues (or stops) as expected; keep using syncService.start(),
syncService.getStatus(), and syncService.stop() while adding appropriate
spies/mocks and assertions for handler invocation and error logging.
In `@apps/backend/tests/integration/booking.test.ts`:
- Around line 48-49: Remove the duplicated assertion that checks the HTTP
status: there are two identical lines calling expect(res.status).toBe(400); —
keep a single expect(res.status).toBe(400); and delete the redundant duplicate
(both reference the same res variable in the failing test).
🧹 Nitpick comments (6)
apps/backend/tests/mocks/supabase.mock.ts (3)
143-143: Unused variableoriginalEq.The variable
originalEqis declared but never used. This appears to be leftover from a refactoring. Consider removing it.🧹 Suggested fix
update: mock(() => { const updateChain = createMockChain(tableName); - const originalEq = updateChain.eq; updateChain.eq = mock((column: string, value: any) => {
165-165: Unused variableoriginalEq.Same issue as above -
originalEqis declared but never used in thedelete()method.🧹 Suggested fix
delete: mock(() => { const deleteChain = createMockChain(tableName); - const originalEq = deleteChain.eq; deleteChain.eq = mock((column: string, value: any) => {
49-78: Filter matching is incomplete forinandmatchoperators.The
single()andthen()methods only handleeq,gt, andltoperators in the filter matching logic, but the chain supportsinandmatchfilters (lines 35-44). If tests use.in()queries, the filtering will silently be skipped.Consider adding support for the
inoperator:💡 Example fix for the filter matching
if (filter.operator === 'lt' && new Date(item[filter.column]) >= new Date(filter.value)) { matches = false; break; } + if (filter.operator === 'in' && !filter.values?.includes(item[filter.column])) { + matches = false; + break; + } }apps/backend/tests/setup.ts (1)
9-44: Consider extracting duplicatedMockServerclass.The
MockServerclass is duplicated for both@stellar/stellar-sdk/rpcand@stellar/stellar-sdk/lib/rpcpaths. While this works, extracting it to a shared constant would reduce duplication:💡 Optional refactor
+class MockServer { + constructor(_url: string) {} + async getLatestLedger() { + return Promise.resolve({ sequence: 1000 }); + } + async getContractEvents() { + return Promise.resolve({ events: [] }); + } +} + mock.module('@stellar/stellar-sdk/rpc', () => { - class MockServer { - constructor(_url: string) {} - async getLatestLedger() { - return Promise.resolve({ sequence: 1000 }); - } - async getContractEvents() { - return Promise.resolve({ events: [] }); - } - } return { Server: MockServer, default: { Server: MockServer }, }; }); mock.module('@stellar/stellar-sdk/lib/rpc', () => { - class MockServer { - constructor(_url: string) {} - async getLatestLedger() { - return Promise.resolve({ sequence: 1000 }); - } - async getContractEvents() { - return Promise.resolve({ events: [] }); - } - } return { Server: MockServer, default: { Server: MockServer }, }; });apps/backend/src/tests/sync.service.test.ts (1)
226-274: Duplicate test cases detected.The tests at lines 226-274 (
should use custom polling interval...andshould fallback to default polling interval...) are exact duplicates of the tests at lines 66-114 within thedescribe('Initialization', ...)block. Consider removing the duplicates.🧹 Suggested fix
}); - - it('should use custom polling interval from environment variable', () => { - // ... duplicate test code - }); - - it('should fallback to default polling interval for invalid environment value', () => { - // ... duplicate test code - }); });apps/backend/tests/integration/booking.test.ts (1)
56-72: Inconsistent language in error messages across the codebase.The auth middleware returns Spanish error messages for 401/403 responses (
"Token no proporcionado","Token inválido o expirado"), while the error middleware and other validators use English ("Invalid token","Token expired","Validation error"). The tests accurately reflect the current implementation, but this mixed-language approach should be addressed:
- Standardize on a single language for all error messages
- Or implement a proper i18n solution if multi-language support is intended
| describe('Event Processing', () => { | ||
| it('should process booking created event', async () => { | ||
| const mockSupabase = supabase as jest.Mocked<typeof supabase>; | ||
|
|
||
| // Mock sync state | ||
| mockSupabase.from.mockReturnValue({ | ||
| select: jest.fn(() => ({ | ||
| single: jest.fn(() => | ||
| Promise.resolve({ | ||
| data: { last_processed_block: 100 }, | ||
| error: null, | ||
| }) | ||
| ), | ||
| })), | ||
| insert: jest.fn(() => Promise.resolve({ data: null, error: null })), | ||
| update: jest.fn(() => ({ | ||
| eq: jest.fn(() => Promise.resolve({ data: null, error: null })), | ||
| })), | ||
| upsert: jest.fn(() => Promise.resolve({ data: null, error: null })), | ||
| } as unknown as ReturnType<typeof supabase.from>); | ||
|
|
||
| const event = { | ||
| id: 'test-event-1', | ||
| type: 'booking_created', | ||
| bookingId: 'booking-123', | ||
| propertyId: 'property-456', | ||
| userId: 'user-789', | ||
| timestamp: new Date(), | ||
| data: { | ||
| escrow_id: 'escrow-123', | ||
| property_id: 'property-456', | ||
| user_id: 'user-789', | ||
| start_date: Math.floor(Date.now() / 1000), | ||
| end_date: Math.floor(Date.now() / 1000) + 86400, | ||
| total_price: 1000, | ||
| status: 'Pending', | ||
| }, | ||
| }; | ||
|
|
||
| // Mock existing booking check | ||
| mockSupabase.from.mockReturnValueOnce({ | ||
| select: jest.fn(() => ({ | ||
| eq: jest.fn(() => ({ | ||
| single: jest.fn(() => Promise.resolve({ data: null, error: null })), | ||
| })), | ||
| })), | ||
| } as unknown as ReturnType<typeof supabase.from>); | ||
|
|
||
| // Mock booking creation | ||
| mockSupabase.from.mockReturnValueOnce({ | ||
| insert: jest.fn(() => Promise.resolve({ data: null, error: null })), | ||
| } as unknown as ReturnType<typeof supabase.from>); | ||
|
|
||
| // Global mock is already set up - this test verifies basic functionality | ||
| await syncService.start(); | ||
|
|
||
| // Simulate event processing | ||
| const processEventMethod = ( | ||
| syncService as unknown as { | ||
| processEvent: (event: Record<string, unknown>) => Promise<void>; | ||
| } | ||
| ).processEvent.bind(syncService); | ||
| await processEventMethod(event); | ||
|
|
||
| expect(mockSupabase.from).toHaveBeenCalledWith('sync_events'); | ||
| expect(mockSupabase.from).toHaveBeenCalledWith('bookings'); | ||
| expect(syncService.getStatus().isRunning).toBe(true); | ||
| await syncService.stop(); | ||
| }); | ||
|
|
||
| it('should handle event processing errors gracefully', async () => { | ||
| const mockSupabase = supabase as jest.Mocked<typeof supabase>; | ||
|
|
||
| // Mock sync state | ||
| mockSupabase.from.mockReturnValue({ | ||
| select: jest.fn(() => ({ | ||
| single: jest.fn(() => | ||
| Promise.resolve({ | ||
| data: { last_processed_block: 100 }, | ||
| error: null, | ||
| }) | ||
| ), | ||
| })), | ||
| insert: jest.fn(() => Promise.reject(new Error('Database error'))), | ||
| update: jest.fn(() => ({ | ||
| eq: jest.fn(() => Promise.resolve({ data: null, error: null })), | ||
| })), | ||
| upsert: jest.fn(() => Promise.resolve({ data: null, error: null })), | ||
| } as unknown as ReturnType<typeof supabase.from>); | ||
|
|
||
| const event = { | ||
| id: 'test-event-2', | ||
| type: 'booking_created', | ||
| bookingId: 'booking-123', | ||
| propertyId: 'property-456', | ||
| userId: 'user-789', | ||
| timestamp: new Date(), | ||
| data: {}, | ||
| }; | ||
|
|
||
| // Global mock is already set up - this test verifies error handling | ||
| await syncService.start(); | ||
|
|
||
| // Simulate event processing | ||
| const processEventMethod = ( | ||
| syncService as unknown as { | ||
| processEvent: (event: Record<string, unknown>) => Promise<void>; | ||
| } | ||
| ).processEvent.bind(syncService); | ||
|
|
||
| // Should throw error due to database failure | ||
| await expect(processEventMethod(event)).rejects.toThrow('Database error'); | ||
|
|
||
| // Failed events count remains unchanged since the error is thrown before marking as failed | ||
| expect(syncService.getStatus().failedEvents).toBe(0); | ||
| expect(syncService.getStatus().isRunning).toBe(true); | ||
| await syncService.stop(); | ||
| }); |
There was a problem hiding this comment.
Test names don't match test behavior.
These tests are named "should process booking created event" and "should handle event processing errors gracefully", but they only verify that the service starts and stops. The tests don't actually verify event processing or error handling behavior.
Consider either:
- Renaming the tests to reflect what they actually test (e.g., "should start and stop service during event processing")
- Implementing actual event processing/error handling assertions
🤖 Prompt for AI Agents
In `@apps/backend/src/tests/sync.service.test.ts` around lines 145 - 158, The test
names claim event processing and error handling but only start/stop the service
(syncService.start(), syncService.getStatus(), syncService.stop()); either
rename the two it(...) descriptions to something like "should start and stop
service during event processing" and "should start and stop service when event
processing errors occur", or expand the tests to assert real behavior: after
syncService.start() emit or call the actual event handler (e.g.,
syncService.processEvent or the event emitter used by the service) with a mock
"booking created" payload and assert the expected handler was invoked and
side-effects occurred, and for the error test stub the handler to throw/reject
and assert the service stays alive, logs the error, and recovers/continues (or
stops) as expected; keep using syncService.start(), syncService.getStatus(), and
syncService.stop() while adding appropriate spies/mocks and assertions for
handler invocation and error logging.
| expect(res.status).toBe(400); | ||
| expect(res.body).toEqual({ | ||
| success: false, | ||
| data: null, | ||
| error: { | ||
| message: 'Bad Request', | ||
| details: expect.any(Object), | ||
| }, | ||
| }); | ||
| expect(res.status).toBe(400); |
There was a problem hiding this comment.
Duplicate assertion.
Line 49 duplicates the status check from line 48.
🧹 Suggested fix
expect(res.status).toBe(400);
- expect(res.status).toBe(400);
expect(res.body.success).toBe(false);📝 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.
| expect(res.status).toBe(400); | |
| expect(res.body).toEqual({ | |
| success: false, | |
| data: null, | |
| error: { | |
| message: 'Bad Request', | |
| details: expect.any(Object), | |
| }, | |
| }); | |
| expect(res.status).toBe(400); | |
| expect(res.status).toBe(400); | |
| expect(res.body.success).toBe(false); |
🤖 Prompt for AI Agents
In `@apps/backend/tests/integration/booking.test.ts` around lines 48 - 49, Remove
the duplicated assertion that checks the HTTP status: there are two identical
lines calling expect(res.status).toBe(400); — keep a single
expect(res.status).toBe(400); and delete the redundant duplicate (both reference
the same res variable in the failing test).
respp
left a comment
respp
left a comment
There was a problem hiding this comment.
Your code is very close to being merged. I’ve left some corrections above; once you address those and follow the coderabbit suggestions, we should be all set
| * Creates a comprehensive Supabase mock that includes all necessary methods | ||
| * with this, the undefined is not an object error is fixed by the auth.getUser method | ||
| */ | ||
| export const createSupabaseMock = () => { |
There was a problem hiding this comment.
The current implementation creates a duplicate mock conflict. Currently, src/config/supabase.ts contains a built-in mock that activates automatically when NODE_ENV === 'test', while the new supabase.mock.ts uses Bun's mock.module() to achieve the same goal. This redundancy can cause unpredictable behavior when setupSupabaseMock() attempts to override a module that is already being mocked conditionally. To resolve this, we should either remove the hardcoded mock from the configuration file in favor of the centralized Bun mock or ensure that the conditional logic in supabase.ts is disabled when the centralized mock is active to avoid initialization conflicts.
There was a problem hiding this comment.
This file does not configure the Supabase mock. Instead, each test handles it individually, which can lead to inconsistencies
There was a problem hiding this comment.
the file is currently too large and monolithic,filtering logic, in-memory storage, and module configuration. To improve maintainability and avoid inconsistencies, this file should be refactored and split into smaller, specialized files:
supabase.mock.tsfor the mock factory,supabase.mock.data.tsfor storage and data,supabase.mock.chain.tsfor the query chain logic,supabase.mock.setup.tsfor module configuration.
| const tableData = mockDataStore[tableName] || new Map(); | ||
| const filters = chain._filters || []; | ||
|
|
||
| // Simple filter matching - find first item that matches all eq filters |
There was a problem hiding this comment.
Isn't this the same logic as the code below? (lines 87-110)
2 participants
Pull Request | StellarRent
📝 Summary
Some Tests were failing because the Supabase mock was not implemented correctly, created a supabase mock file and updated tests to use it. All tests that use this mock are passing.
🔗 Related Issues
Closes #197
🔄 Changes Made
Fixed Supabase mock configuration across all test files to resolve "undefined is not an object (evaluating 'supabase.auth.getUser')" errors.
🖼️ Current Output
🧪 Testing
Screenshots above.
✅ Testing Checklist
Most tests outside of the ones done for this PR still fail, so it is good to take a look at them.
🚀 Next Steps & Improvements
This change lays a solid foundation for further optimizations. Some areas that could benefit from future improvements include:
💬 Comments
Thanks for reviewing this PR! If you see that I made more files than required, it is because I make the Supabase Mock separated and updated files that needed changes.
Summary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.