Skip to content
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

Implement automated code coverage for CI #362

Merged
merged 4 commits into from
Nov 6, 2023
Merged

Conversation

tiffzhao5
Copy link
Collaborator

@tiffzhao5 tiffzhao5 commented Nov 6, 2023

Pull Request Summary

Used diff-cover for automated code coverage. In my temp commit, I wrote an endpoint that wasn't covered by unit tests and this is what I got:
image

Test Plan and Usage Guide

How did you validate that your PR works correctly? How do you run or demo the code? Provide enough detail so a reviewer can reasonably reproduce the testing procedure. Paste example command line invocations if applicable.

Copy link
Member

@yixu34 yixu34 left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! Much needed. Can you paste the initial output from this in the PR description? Just to give us an idea of how much ground to 80% we need to cover before CI is green again 🟢

@tiffzhao5
Copy link
Collaborator Author

Thanks for doing this! Much needed. Can you paste the initial output from this in the PR description? Just to give us an idea of how much ground to 80% we need to cover before CI is green again 🟢

@yixu34 Pasted the initial output! Also, just to clarify this PR is only checking that the code coverage of the incremental changes, not the entire llm-engine repo, is 80%. Let me know if this is 80% is too high -- alternatively, I was thinking of adding CI PR comments instead of failing CI.

@yunfeng-scale
Copy link
Collaborator

Thanks for doing this! Much needed. Can you paste the initial output from this in the PR description? Just to give us an idea of how much ground to 80% we need to cover before CI is green again 🟢

@yixu34 Pasted the initial output! Also, just to clarify this PR is only checking that the code coverage of the incremental changes, not the entire llm-engine repo, is 80%. Let me know if this is 80% is too high -- alternatively, I was thinking of adding CI PR comments instead of failing CI.

it'll be nice to add an extra step on master branch to get coverage for all files

@yixu34
Copy link
Member

yixu34 commented Nov 6, 2023

Thanks for doing this! Much needed. Can you paste the initial output from this in the PR description? Just to give us an idea of how much ground to 80% we need to cover before CI is green again 🟢

@yixu34 Pasted the initial output! Also, just to clarify this PR is only checking that the code coverage of the incremental changes, not the entire llm-engine repo, is 80%. Let me know if this is 80% is too high -- alternatively, I was thinking of adding CI PR comments instead of failing CI.

Nope failing CI sounds good! Especially since it's only incremental for now.

it'll be nice to add an extra step on master branch to get coverage for all files

Agree, this can be a follow-up?

@tiffzhao5 tiffzhao5 merged commit 751bf38 into main Nov 6, 2023
5 checks passed
@tiffzhao5 tiffzhao5 deleted the tiffany/testcoverage branch November 6, 2023 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants