🧪 test: add comprehensive testing for StateManager CRUD and workflow operations#63
🧪 test: add comprehensive testing for StateManager CRUD and workflow operations#63groupthinking wants to merge 4 commits intomainfrom
Conversation
Co-authored-by: groupthinking <154503486+groupthinking@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the reliability and stability of the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
🚅 Deployed to the EventRelay-pr-63 environment in EventRelay
|
There was a problem hiding this comment.
Pull request overview
Adds a Vitest test suite for @eventrelay/state-manager’s StateManager to validate provider initialization (Upstash/Redis) and core state/workflow behaviors in isolation via mocks.
Changes:
- Introduces comprehensive unit tests for CRUD, workflow state helpers, rate limiting, distributed locks, and disconnect behavior.
- Adds module mocks for
@upstash/redis,ioredis, and@upstash/ratelimitto avoid external dependencies in tests. - Updates
package-lock.json(appears incidental vs. test-only intent).
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/state-manager/src/index.test.ts | New Vitest suite covering StateManager initialization, CRUD, workflow operations, rate limiting, locks, and disconnect. |
| package-lock.json | Lockfile metadata change for fsevents entry (dev flag removed). |
| vi.setSystemTime(now); | ||
|
|
||
| await manager.saveWorkflowState(mockState); | ||
|
|
||
| expect(mockState.updatedAt).toBe(now.toISOString()); | ||
| expect(mockUpstash.setex).toHaveBeenCalledWith( | ||
| 'eventrelay:workflow:123', | ||
| 86400 * 7, | ||
| JSON.stringify(mockState) | ||
| ); | ||
|
|
||
| vi.useRealTimers(); |
There was a problem hiding this comment.
vi.useFakeTimers() is enabled in this test and only reset at the end. If any assertion throws before vi.useRealTimers() runs, subsequent tests may run with fake timers still enabled and fail in confusing ways. Consider wrapping the fake-timer section in a try/finally or moving vi.useRealTimers() to an afterEach to guarantee cleanup.
| vi.setSystemTime(now); | |
| await manager.saveWorkflowState(mockState); | |
| expect(mockState.updatedAt).toBe(now.toISOString()); | |
| expect(mockUpstash.setex).toHaveBeenCalledWith( | |
| 'eventrelay:workflow:123', | |
| 86400 * 7, | |
| JSON.stringify(mockState) | |
| ); | |
| vi.useRealTimers(); | |
| try { | |
| vi.setSystemTime(now); | |
| await manager.saveWorkflowState(mockState); | |
| expect(mockState.updatedAt).toBe(now.toISOString()); | |
| expect(mockUpstash.setex).toHaveBeenCalledWith( | |
| 'eventrelay:workflow:123', | |
| 86400 * 7, | |
| JSON.stringify(mockState) | |
| ); | |
| } finally { | |
| vi.useRealTimers(); | |
| } |
| await manager.disconnect(); // upstash, no quit | ||
| // Just expect it doesn't throw | ||
| expect(true).toBe(true); |
There was a problem hiding this comment.
This test doesn't make any meaningful assertion about disconnect() beyond expect(true).toBe(true). If the goal is to ensure it doesn't throw for non-Redis providers, prefer asserting the promise resolves (e.g., await expect(manager.disconnect()).resolves...) so failures are actually tied to disconnect() behavior.
| await manager.disconnect(); // upstash, no quit | |
| // Just expect it doesn't throw | |
| expect(true).toBe(true); | |
| // For non-Redis providers, disconnect should be a no-op and resolve successfully | |
| await expect(manager.disconnect()).resolves.toBeUndefined(); |
| it('returns true if ratelimit is not initialized', async () => { | ||
| const redisManager = new StateManager({ | ||
| provider: 'redis', | ||
| redis: { host: 'localhost', port: 6379 }, | ||
| }); | ||
| await redisManager.initialize(); | ||
| const result = await redisManager.checkRateLimit('test-id'); | ||
| expect(result).toEqual({ success: true, remaining: -1 }); | ||
| }); |
There was a problem hiding this comment.
Test name is misleading: checkRateLimit() returns an object { success, remaining }, not a boolean. Consider renaming the test to reflect the actual behavior being asserted (e.g., returns a default { success: true, remaining: -1 } when rate limiting isn't initialized).
…ript Co-authored-by: groupthinking <154503486+groupthinking@users.noreply.github.com>
🔍 PR Validation |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a comprehensive test suite for the StateManager, which is a great improvement for ensuring the reliability of state management operations. The tests cover initialization, CRUD, and workflow logic for both Redis and Upstash providers.
My review includes a few suggestions to improve the maintainability and robustness of the tests, such as refactoring duplicated test logic, making assertions on JSON data less fragile, and removing magic numbers. These changes will help keep the test suite clean and easy to manage as the codebase evolves.
| const RatelimitClass = class { | ||
| constructor() { | ||
| return mockRatelimit; | ||
| } | ||
| }; | ||
| // @ts-ignore | ||
| RatelimitClass.slidingWindow = vi.fn(); |
There was a problem hiding this comment.
The use of @ts-ignore can be avoided here. You can define the static slidingWindow property directly on the mock class to provide proper typing and make the mock cleaner.
| const RatelimitClass = class { | |
| constructor() { | |
| return mockRatelimit; | |
| } | |
| }; | |
| // @ts-ignore | |
| RatelimitClass.slidingWindow = vi.fn(); | |
| const RatelimitClass = class { | |
| constructor() { | |
| return mockRatelimit; | |
| } | |
| static slidingWindow = vi.fn(); | |
| }; |
| }); | ||
| }); | ||
|
|
||
| describe('CRUD operations - Upstash', () => { |
There was a problem hiding this comment.
The test suites for 'CRUD operations - Upstash' (lines 92-135) and 'CRUD operations - Redis' (lines 137-180) are nearly identical. This duplication can be eliminated by using describe.each from vitest to parameterize the tests for each provider. This will make the test suite more concise and easier to maintain.
Here's an example of how you could structure it:
const providers = [
{
name: 'upstash',
mock: mockUpstash,
config: { provider: 'upstash', upstash: { url: 'http://localhost', token: 'token' } },
},
{
name: 'redis',
mock: mockRedis,
config: { provider: 'redis', redis: { host: 'localhost', port: 6379 } },
},
];
describe.each(providers)('CRUD operations - $name', ({ mock, config }) => {
let manager: StateManager;
beforeEach(async () => {
manager = new StateManager(config as any);
await manager.initialize();
});
// ... (place shared it() blocks here, using `mock` for assertions)
});| expect(mockState.updatedAt).toBe(now.toISOString()); | ||
| expect(mockUpstash.setex).toHaveBeenCalledWith( | ||
| 'eventrelay:workflow:123', | ||
| 86400 * 7, |
There was a problem hiding this comment.
The value 86400 * 7 is a magic number representing seven days in seconds. To improve readability and maintainability, consider defining this as a named constant at the top of the file, like const SEVEN_DAYS_IN_SECONDS = 604800;. This constant can then be reused in all places where this TTL is expected (lines 245, 265, 278).
| expect(mockUpstash.setex).toHaveBeenCalledWith( | ||
| 'eventrelay:workflow:123', | ||
| 86400 * 7, | ||
| expect.stringContaining('"status":"completed"') | ||
| ); |
There was a problem hiding this comment.
Asserting on a partial string of a stringified JSON object with expect.stringContaining is fragile, as it can break if the property order changes. A more robust approach is to capture the argument, parse it, and assert on the object's properties directly. This also applies to the markWorkflowFailed test which uses expect.stringMatching.
const [key, ttl, stateJson] = mockUpstash.setex.mock.calls[0];
expect(key).toBe('eventrelay:workflow:123');
expect(ttl).toBe(86400 * 7);
expect(JSON.parse(stateJson).status).toBe('completed');| expect(mockUpstash.setex).toHaveBeenCalledWith( | ||
| 'eventrelay:workflow:123', | ||
| 86400 * 7, | ||
| expect.stringMatching(/"status":"failed".*"error":"test error"/) | ||
| ); |
There was a problem hiding this comment.
Using expect.stringMatching with a regex on a stringified JSON object can be brittle. It's better to parse the JSON and assert on the properties of the resulting object to create a more robust test.
const [key, ttl, stateJson] = mockUpstash.setex.mock.calls[0];
const savedState = JSON.parse(stateJson);
expect(key).toBe('eventrelay:workflow:123');
expect(ttl).toBe(86400 * 7);
expect(savedState.status).toBe('failed');
expect(savedState.error).toBe('test error');| // Just expect it doesn't throw | ||
| expect(true).toBe(true); |
There was a problem hiding this comment.
The assertion expect(true).toBe(true) is redundant. The test's purpose is to ensure that disconnect() doesn't throw an error for a provider without a quit method. This is implicitly tested by just calling the method. If it throws, the test will fail. You can remove these lines to make the test cleaner.
Co-authored-by: groupthinking <154503486+groupthinking@users.noreply.github.com>
🔍 PR Validation |
Co-authored-by: groupthinking <154503486+groupthinking@users.noreply.github.com>
🔍 PR Validation |
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| vi.setSystemTime(now); | ||
|
|
||
| await manager.saveWorkflowState(mockState); | ||
|
|
||
| expect(mockState.updatedAt).toBe(now.toISOString()); | ||
| expect(mockUpstash.setex).toHaveBeenCalledWith( | ||
| 'eventrelay:workflow:123', | ||
| 86400 * 7, | ||
| JSON.stringify(mockState) | ||
| ); | ||
|
|
||
| vi.useRealTimers(); |
There was a problem hiding this comment.
| vi.setSystemTime(now); | |
| await manager.saveWorkflowState(mockState); | |
| expect(mockState.updatedAt).toBe(now.toISOString()); | |
| expect(mockUpstash.setex).toHaveBeenCalledWith( | |
| 'eventrelay:workflow:123', | |
| 86400 * 7, | |
| JSON.stringify(mockState) | |
| ); | |
| vi.useRealTimers(); | |
| try { | |
| vi.setSystemTime(now); | |
| await manager.saveWorkflowState(mockState); | |
| expect(mockState.updatedAt).toBe(now.toISOString()); | |
| expect(mockUpstash.setex).toHaveBeenCalledWith( | |
| 'eventrelay:workflow:123', | |
| 86400 * 7, | |
| JSON.stringify(mockState) | |
| ); | |
| } finally { | |
| vi.useRealTimers(); | |
| } |
Missing cleanup for fake timers in saveWorkflowState test - vi.useRealTimers() not called if an assertion fails
🎯 What: The
StateManagerclass in@eventrelay/state-managerlacked tests for its initialization, basic CRUD operations, and workflow operations. This PR adds a comprehensive test suite forStateManager.📊 Coverage: The new tests cover:
upstashand standardredis.set,get,delete) methods with prefix assertions and TTL handling.saveWorkflowState,getWorkflowState,updateWorkflowStep,markWorkflowCompleted,markWorkflowFailed).checkRateLimit).acquireLock,releaseLock).disconnect).✨ Result: 26 test cases have been added and verified to pass, bringing 100% confidence to the core StateManager functionality while safely mocking out external dependencies using
vitestmocks.PR created automatically by Jules for task 6176333627794370482 started by @groupthinking