-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add missing unit tests #9
Conversation
…Cow Explorer builds fine
ESLint Summary View Full Report
[failure] @typescript-eslint/no-explicit-any
Report generated by eslint-plus-action |
I'm not pretty sure why Coveralls BOT is not commenting the 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.
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 are getting there!
Apart from the code comments, I'd also add some tests to validate that the passed appDataHash
on the SDK's constructor is actually set on the X-AppId
header
Yeah, the idea is to have our features tested as long as we can. We have reached around 90% in CowApi with this PR. We should also trying to achieve the same for the others APIs (It could be achieved in follow up PRs). Also, it is a good practice to create unit tests with new features (The PR of a new feature should include the logic and also the tests) to keep the coverage. |
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.
🚢
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 job!
Left a few nitpicks and questions but it's approved already
This PR closes #2