test: migrate to MSW and document testing best practices #129
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Replace spyOn(globalThis, 'fetch') with MSW in feedback.spec.ts for better consistency with other tests Changes: - Add MSW handler for StackOne AI tool feedback endpoint in mocks/handlers.ts - Migrate all fetch mocks in src/tools/tests/feedback.spec.ts to use MSW - Update CLAUDE.md with comprehensive MSW testing guidance - Update .cursor/rules/bun-test-mocks.mdc with MSW best practices MSW provides better consistency across tests and is already configured globally in bun.test.setup.ts. Note: One pre-existing test failure in validation (whitespace-only tool_names) is unrelated to this change.
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the feedback tool tests from using spyOn(globalThis, 'fetch') to MSW (Mock Service Worker) for HTTP request mocking, aligning with the testing patterns already established in other test files in the codebase.
Key changes:
- Replaced all
spyOn(globalThis, 'fetch')calls with MSW event listeners and handler overrides - Added a new MSW handler for the StackOne AI tool feedback endpoint
- Updated documentation (CLAUDE.md and Cursor rules) with comprehensive MSW testing guidance
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/tools/tests/feedback.spec.ts | Migrated all test cases from fetch mocking to MSW patterns using event listeners and handler overrides |
| mocks/handlers.ts | Added default MSW handler for the StackOne AI tool feedback endpoint |
| CLAUDE.md | Added new "Testing with MSW" section with examples and best practices |
| .cursor/rules/bun-test-mocks.mdc | Added new "HTTP Request Mocking with MSW" section with guidance for Cursor users |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Comment on lines
+213
to
+222
| let callCount = 0; | ||
| server.use( | ||
| http.post('https://api.stackone.com/ai/tool-feedback', () => { | ||
| callCount++; | ||
| if (callCount === 1) { | ||
| return HttpResponse.json({ message: 'Success' }); | ||
| } | ||
| return HttpResponse.json({ error: 'Unauthorized' }, { status: 401 }); | ||
| }) | ||
| ); |
There was a problem hiding this comment.
The callCount variable is shared across potentially concurrent requests and is not reset after the test. If the handler is reused or requests execute in parallel, this could lead to incorrect behavior. Consider resetting callCount inside the test or using a more robust pattern like checking request properties to determine the response.
commit: |
Add comprehensive fs-fixture documentation for file system testing: - Basic usage with await using syntax for automatic cleanup - Pre-populated fixtures for test setup - Type-safe JSON operations with generics - Complete API reference for fixture methods - Benefits and best practices Updated both CLAUDE.md and .cursor/rules/bun-test-mocks.mdc
Make await using syntax mandatory for fs-fixture usage: - Add prominent warning to always use await using - Remove fixture.rm() from API documentation to discourage manual cleanup - Add direct link to official fs-fixture documentation for reference - Emphasize automatic cleanup as the primary benefit
ryoppippi
changed the title
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Add validation to ensure tool_names array is not empty after trimming and filtering whitespace-only strings. Previously, tool_names like [' ', ' '] would be transformed to [] but not rejected, allowing invalid requests to be sent. This adds a .refine() check after the .transform() to ensure at least one non-empty tool name remains after filtering.
…ests-to-msw Include validation fix discovered during MSW migration
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related to #125
now we need to use msw and fs-fixture instead of mocking functions
Summary
Migrates feedback tests to MSW, adds comprehensive testing documentation, and fixes a validation bug discovered during migration.
Changes
MSW Migration:
feedback.spec.tsfromspyOn(globalThis, 'fetch')to MSWBug Fix (from #130):
tool_namesvalidation bug where whitespace-only strings were transformed to[]without errorDocumentation:
await usingsyntaxCLAUDE.mdand.cursor/rules/bun-test-mocks.mdcTest Results
✅ All 10 feedback tests pass
Merge Order
Why MSW?
bun.test.setup.tsspyOn(globalThis, 'fetch')