Skip to content

Conversation

@maifeeulasad
Copy link

Description & Motivation

partial solution of #270

Related Issue(s)

#270

Testing

  • Ran the script locally
  • Couldnt run in my repo, due to some runner limitation I guess. It was stuck

Backwards-compatibility

Completly, as it is new feature

Is this a breaking change that will not be backwards-compatible? If yes, how so?
Non breaking change, as it is a new feature, working on a completely different branch

Documentation

Does the change require any updates to documentation? If so, where? Are they included?

Only one created script is documented inside, nothing more than that.

Copy link
Contributor

@mkorbel1 mkorbel1 left a 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
Copy link
Contributor

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?

Copy link
Author

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 run is deprecated, i think we should just use dart run, for our case dart test is 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%"
Copy link
Contributor

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?

Copy link
Author

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
Copy link
Contributor

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?

Copy link
Author

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

@maifeeulasad
Copy link
Author

@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 coverage branch. And the main readme file will point to that svg somewhere and we will get a beautiful badge there. This is the strategy for main branch.

@maifeeulasad maifeeulasad requested a review from mkorbel1 January 26, 2026 12:51
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.

2 participants