-
Notifications
You must be signed in to change notification settings - Fork 3
test: add error handling and edge case tests, improve coverage 85%→93% #261
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
Conversation
- Split StackOneError into error-stackone.ts - Split StackOneAPIError into error-stackone-api.ts - Remove re-export file errors.ts - Update all imports to use direct paths Improves searchability and code organisation.
- Add comprehensive tests for StackOneError and StackOneAPIError - Add tests for BaseTool RPC/local config and error handling - Add tests for StackOneToolSet dryRun mode and parameter extraction - Exclude type-only files from coverage (index.ts, type.ts) Coverage improvements: - Statements: 85.29% → 92.50% - Branch: 70.61% → 84.05% - Lines: 85.95% → 93.51%
commit: |
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.
No issues found across 18 files
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.
Pull request overview
This PR improves test coverage from 85% to 93% and refactors error classes into separate files for better code organization and maintainability.
- Splits error classes into dedicated files (
error-stackone.tsanderror-stackone-api.ts) - Adds comprehensive test coverage for error classes and tool execution paths
- Excludes type-only files from coverage metrics to focus on executable code
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| vitest.config.ts | Excludes type-only files (index.ts, type.ts) from coverage calculation |
| src/utils/error-stackone.ts | New file containing base StackOneError class |
| src/utils/error-stackone-api.ts | Refactored to import StackOneError from separate file instead of defining it locally |
| src/utils/error-stackone.test.ts | New comprehensive tests for StackOneError class |
| src/utils/error-stackone-api.test.ts | New comprehensive tests for StackOneAPIError class including toString formatting |
| src/toolsets.test.ts | Added tests for dryRun mode, parameter extraction, and error handling |
| src/tool.test.ts | Added tests for RPC/local config metadata, execution options, and error handling |
| src/utils/try-import.ts | Updated import path from ./errors to ./error-stackone |
| src/utils/try-import.test.ts | Updated import path from ./errors to ./error-stackone |
| src/toolsets.ts | Updated import path from ./utils/errors to ./utils/error-stackone |
| src/tool.ts | Updated import path from ./utils/errors to ./utils/error-stackone |
| src/rpc-client.ts | Updated import path from ./utils/errors to ./utils/error-stackone-api |
| src/rpc-client.test.ts | Updated import path from ./utils/errors to ./utils/error-stackone-api |
| src/requestBuilder.ts | Updated import path from ./utils/errors to ./utils/error-stackone-api |
| src/requestBuilder.test.ts | Updated import path from ./utils/errors to ./utils/error-stackone-api |
| src/index.ts | Updated exports to import from new separate error files |
| src/feedback.ts | Updated import path from ./utils/errors to ./utils/error-stackone |
| src/feedback.test.ts | Updated import path from ./utils/errors to ./utils/error-stackone |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/utils/error-stackone-api.test.ts
Outdated
|
|
||
| it('should include endpoint URL when present in message', () => { | ||
| const error = new StackOneAPIError( | ||
| 'Request failed for https://api.stackone.com/unified/hris/employees', |
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.
I don't think we should have unified endpoints related tests
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.
oh yes you are absolutely right!
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.
Apply review feedback from @glebedel - this SDK should not have unified endpoints related tests. Changed the test URL from /unified/hris/employees to /tools/execute.
glebedel
left a comment
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.
LGTM
Summary
Improves test coverage from 85% to 93% and refactors error classes into separate files for better code organisation and searchability.
What Changed
Refactoring:
errors.tsintoerror-stackone.tsanderror-stackone-api.tsTest Coverage:
StackOneErrorandStackOneAPIErrorBaseToolRPC/local config and error handlingStackOneToolSetdryRun mode and parameter extractionindex.ts,type.ts)Coverage Improvements
Testing
All 427 tests pass across 32 test files.
Summary by cubic
Boosts test coverage to ~93% and splits error classes into dedicated files for clearer imports and easier search.
Refactors
Test Coverage
Written for commit 9b56837. Summary will update automatically on new commits.