-
Notifications
You must be signed in to change notification settings - Fork 41
chore: add type-safe ESLint #119
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
1631038
to
003fe9c
Compare
003fe9c
to
7164709
Compare
@@ -203,8 +203,9 @@ export function validateThrowsForInvalidArguments( | |||
it(`throws a schema error for: ${JSON.stringify(arg)}`, async () => { | |||
try { | |||
await integration.mcpClient().callTool({ name, arguments: arg }); | |||
expect.fail("Expected an error to be thrown"); | |||
throw new Error("Expected an error to be thrown"); |
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.
this was not working correctly so we had the false impression that extra fields are problematic with our zod setup; they're not so I removed places where we were using that as an invalid arg scenario
@@ -8,7 +8,7 @@ | |||
"strict": true, | |||
"strictNullChecks": true, | |||
"esModuleInterop": true, | |||
"types": ["node"], | |||
"types": ["node", "jest"], |
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.
This just seems to be the best way to get VSCode to stop complaining about the types once in a while but I think can work without too as long as others are represented
Pull Request Test Coverage Report for Build 14665235990Details
💛 - Coveralls |
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.
Nice!
} catch (error) { | ||
expect((error as Error).message).not.toEqual("Expected an error to be thrown"); |
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.
Isn't the assertion just below sufficient? I don't imagine Error
is an instance of McpError
.
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.
it is but then the error message is more confusing (it just says expected McpError
got Error
which originally made me think it was erroring differently instead of not erroring at all). I want to differentiate an external error from the intentional "did not end up erroring" error
c239276
to
140830c
Compare
Fixes #113
Largely did the minimum viable actions needed to get the linting to pass.
Also added
expectDefined
so we don't need to keep asserting our types manually.