-
Notifications
You must be signed in to change notification settings - Fork 80
Add automation for code coverage in pushes #646
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
base: main
Are you sure you want to change the base?
Conversation
mkorbel1
left a comment
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.
This looks great! Do you want to try running this on your fork (update the runner so it uses the GitHub-hosted ones so it runs for you) to demo that it works?
| rm -rf coverage | ||
|
|
||
| # Run tests with coverage | ||
| dart test --coverage=coverage || true |
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.
this runs coverage collection differently than the other script (https://github.com/intel/rohd/blob/main/tool/generate_coverage.sh), do you know if this approach is a newer/better way to collect coverage? should we update the other script? should these scripts be merged into one or one calls another instead of independent?
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 forgot to generate branch coverage, fixing that right away
dart pub global runis deprecated, i think we should just usedart run, for our casedart testis just fine
I am willing to make modifications I just didn't know what that file was being used for that's why created this script completly bypassing that and generating svg along with it. Yeah, we can pass in some additional argument to tweak it's behavior. Wating to hear back from you!
ref: dart-lang/pub#3066
| echo "Coverage: ${PERCENT}%" | ||
| else | ||
| PERCENT="0.0" | ||
| echo "Coverage: 0.0%" |
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.
would an error message be better there instead of just 0?
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.
Care to suggest one please? I will update the PR accordingly
|
|
||
| # Generate SVG badge | ||
| # Style credit goes to shields.io | ||
| cat > /tmp/coverage-badge.svg << EOFSVG |
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.
question: why make our own SVG instead of using something like https://shields.io/ like we use for the README?
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.
- it will push our github metrics up, ref: https://github.com/intel/rohd/graphs/traffic
- i like keeping 3rd party as much out as possible, that's why i kept it out. But I don't don't have anything against shields or badge io
- and if we use shields io, we need to keep push it to that branch, and write a railgruard to disable infinite action glitch
but if you want i can make adjustment for that
|
@mkorbel1 I pushed to a different branch for testing and yes it is working as it should
So basically the strategy is to generate report for each PR for main and push to that |
Description & Motivation
partial solution of #270
Related Issue(s)
#270
Testing
Backwards-compatibility
Completly, as it is new feature
Documentation
Only one created script is documented inside, nothing more than that.