-
Notifications
You must be signed in to change notification settings - Fork 87
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
feat: local provider scheduled command adapter unit test #1095
feat: local provider scheduled command adapter unit test #1095
Conversation
Hi @gomarcopololead, thanks for the Pull Request, could you please add a brief description of what you did and why to help us better understand your change? Thanks!! Also feel free to join our Discord channel and ask any questions there: https://discord.gg/bDY8MKx |
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.
Hi @gomarcopololead I left a few suggestions and ideas. Please take into account that some new commits have been added to main
since you open this PR that might affect your code, so you might want to rebase it to make sure that tests still work. Thanks!
packages/framework-provider-local-infrastructure/src/scheduler.ts
Outdated
Show resolved
Hide resolved
packages/framework-provider-local-infrastructure/test/scheduler.test.ts
Outdated
Show resolved
Hide resolved
packages/framework-provider-local-infrastructure/test/scheduler.test.ts
Outdated
Show resolved
Hide resolved
This PR is for framework-provider-local scheduled-adapter unit test and framework-provider-local-infrastructure scheduler unit test. To test scheduler Overall this test helps to make sure schedule command works correctly at local-provider environment. |
Hey, thanks for the submission! 😄 One question, any reason that we created only the local provider's tests and not for the rest of the providers? Thanks again! |
@javiertoledo Hey @NickSeagull |
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 the changes, your code looks good! I think that we could add a few more tests to check a wider range of scenarios, but that can be done in a new PR.
Hey @gomarcopololead looks like the unit tests are failing, can you run |
bcad4f1
to
672334e
Compare
I've moved the commits from this branch to a new PR on top of the current |
Description
Changes
Checks