-
Notifications
You must be signed in to change notification settings - Fork 405
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 more tests for caliper worker #1625
Add more tests for caliper worker #1625
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.
Sorry I think this requires a bit of rework. Please don't fall into the trap that you are writing tests that should pass the current implementation only. The approach should be to write tests of the behaviour you expect from the code and then make sure it passes which could result in a needed code change. These tests should find bugs as well as ensure it's easy to make changes and to check the behaviour remains the same
486e0b3
to
d4a42ab
Compare
c99470a
to
36a2cb2
Compare
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.
Apologies I missed the fact that the worker stops on the first error so we don't need to make the changes to the error handling
36a2cb2
to
a278abf
Compare
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.
Still some issues I'm afraid
a278abf
to
e20dfcc
Compare
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.
A few minor things plus ideally we could use some more tests but at least this is more than before
e20dfcc
to
e678fb2
Compare
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.
some final minor comments.
Signed-off-by: Babatunde Sanusi <swisskid95@gmail.com>
e678fb2
to
a50f342
Compare
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 @tunedev
Checklist
Issue/User story
Partially addresses #1606
Steps to Reproduce
Existing issues
Design of the fix
Validation of the fix
After this PR: Running the tests in caliper-core the listed %stmts in the code coverage report should tally with the below listed
Before the PR: Running the tests in caliper-core the listed %stmts in the code coverage report the below listed was what was there before
Automated Tests
What documentation has been provided for this pull request