-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
test: adding tests. changed files actions.test.ts, messages.test.ts, models.test.ts #998
Conversation
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.
Thanks for doing this looks good outside of comments :)
handler: async () => { throw new Error("Not implemented"); }, | ||
validate: async () => { throw new Error("Not implemented"); }, | ||
}, |
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.
We should probably implement these and test then properly in a follow up PR :)
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.
Can't wait! :)
|
||
test("should have correct model mappings", () => { | ||
const openAIModels = models[ModelProviderName.OPENAI].model; | ||
expect(openAIModels[ModelClass.SMALL]).toBe("gpt-4o-mini"); |
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.
instead of hardcoding we pay want to use the settings and then default to the hard coded values. As this will change for instance when we add o1 and o1 pro support See https://github.com/ai16z/eliza/pull/999/files#diff-c128c91a940e645cc1d7842bec41d065604fce9e1bbc69dfca3f3d089cac52eb
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.
Good point
Please update the PR to be against develop and not main |
Changed base to develop @monilpat |
Relates to:
No particular issue, adding more coverage and more tests for actions.test.ts, messages.test.ts, models.test.ts to cover more functionalities.
Risks
Low, adding tests.
Background
What does this PR do?
This PR adds more tests to already existing test suites.
What kind of change is this?
Documentation changes needed?
Testing
Where should a reviewer start?
packages/core/src/tests/
Detailed testing steps
navigate to packages/core and run pnpm install and pnpm test