Skip to content

Conversation

yize415
Copy link
Contributor

@yize415 yize415 commented Apr 25, 2023

*Issue number of the reported bug or feature request: #51 *

Describe your changes

  1. More tests are introduced in this PR such that make pycoverage returns 100% coverage rate.
  2. Coverage requirement is bumped from 97% to 100%.
  3. Test files are no longer excluded from coverage report.

Testing performed
make check

Additional context

@yize415 yize415 changed the title Cover100 Reach 100% python coverage rate Apr 25, 2023
@godlygeek
Copy link
Contributor

godlygeek commented Apr 25, 2023

@yize415 Thanks, this looks awesome! Would you mind editing your commit messages to add the DCO signoff? You can squash both of these into one commit at the same time, too.

From the state your PR is in now, you can run:

git reset --soft d2b425be1389bf151043c366212a12a3d437f27a

to revert your commits but leave your changes staged, and then you can run:

git commit -s -m "tests: Reach 100% Python code coverage"

to re-commit your changes as a single commit and with the DCO signoff added.

And after that you'd need to force-push those changes back to the PR, with:

git push -f

@yize415
Copy link
Contributor Author

yize415 commented Apr 25, 2023

Thanks for instruction! I just force-pushed and let me know if further actions are needed

@pablogsal
Copy link
Member

@yize415 Unfortunately some of the latest merges have created conflicts with this PR. Would you mind solving them so we can merge? If you have problems you can ping me (@pablogsal ) or ask @godlygeek for help!

@yize415
Copy link
Contributor Author

yize415 commented Apr 26, 2023

Just solved merge conflicts and rebased!

Signed-off-by: Yize Xie <yize415@gmail.com>
Copy link
Contributor

@godlygeek godlygeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@godlygeek godlygeek merged commit 320414f into bloomberg:main Apr 27, 2023
@godlygeek
Copy link
Contributor

Thanks so much for the contribution, @yize415!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants