-
Notifications
You must be signed in to change notification settings - Fork 29.1k
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
test_runner: add coverage support to run function #53937
test_runner: add coverage support to run function #53937
Conversation
Review requested:
|
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.
Left a couple comments regarding a possible longer term strategy, but this is looking good. Thank you for picking this up!
e9d7a22
to
05f4e39
Compare
2907841
to
7ef36f2
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.
I think these changes are moving in the right direction, but I think this should be split up into multiple PRs.
@atlowChemi just a heads up if you come back to this PR - there will likely be new flags to account for from #54429. A lot of internal refactoring also happened, so the diff here should be a lot simpler. |
@cjihrig Thanks for the heads up! I am trying yo get back to this, been pretty busy in work & personal life lately, hoping to get back to this soon 🙏🏽 |
7ef36f2
to
9a9c295
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.
Other than docs, this looks pretty much done!
This comment was marked as resolved.
This comment was marked as resolved.
Do @RedYetiDev's comments make a difference? If they still fail with those changes, I can take a look. |
They do! Thanks @RedYetiDev 🙂 |
4ab6491
to
ffe1e56
Compare
🎉 Looks good imo! |
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.
lgtm
|
7f4608c
to
db37754
Compare
Was using posix separator 🙂 |
Landed in f79fd03 |
Fixes: #53867
Refs: #53924