-
Notifications
You must be signed in to change notification settings - Fork 146
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
refactor(metrics): unit tests for Metrics are written to follow similar convention as Logger/Tracer #1414
refactor(metrics): unit tests for Metrics are written to follow similar convention as Logger/Tracer #1414
Conversation
…hed max metric size threshold
…hed max metric size threshold
…eaches maximum allowed
…OnEmptyMetrics is set to true
… in constructor or environment variable
This will help visualize the output of serializeMetrics for different inputs
Hi @arnabrahman thank you so much for opening this PR! Given the size, please allow us some time to go through it and review it. I'll get back to you here with comments as I review it. |
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've done a first review and things are looking good!
I have left one comment about the helper function, that I think we could drop.
Now I'm going to focus on reviewing the actual test cases and see if all the previous ones are covered.
This helper will be removed/deprecated from all utilities(i.e. createLogger) in the next major release. So, avoiding it is logical.
Removing createMetrics helper function and related tests impacts 100% code coverage. So, instead createMetrics tests can be used as constructor method tests for Metrics class. As a result 100% test coverage will not be impacted.
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 @arnabrahman thank you so much for the work you've done here.
I really appreciate the effort and dedication you've put into improving the project.
Happy to help |
Description of your changes
Metrics unit tests were not following the same convention as Logger/Tracer package. In Logger/Tracer tests, only public functions are unit tested. But for Metrics, unit tests are written per feature/behavior.
This PR updates the tests to match the same convention as Logger/Tracer package. Only public methods are tested, following the recommendations here: Best Practices
100% test coverage is maintained. Some additional housekeeping is implemented,
constants
&helpers
are introduced for better code maintainability.I've removed the previous feature tests because in my understanding leaving them will create further confusion about the testing patterns. I can add them again if maintainers feel the current sets of tests in this PR are inadequate.
How to verify this change
All the tests will run successfully along with 100% test coverage.
Related issues, RFCs
Issue number: #163
Checklist
Breaking change checklist
Is it a breaking change?: NO
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.