allow Coverage to start when Coverage.running? is not implemented#1043
Conversation
eregon
left a comment
eregon
left a comment
There was a problem hiding this comment.
This looks good to me, thank you!
|
|
||
| def coverage_running? | ||
| Coverage.running? | ||
| rescue NoMethodError |
There was a problem hiding this comment.
if Coverage.respond_to? :running? seems nicer than rescue NoMethodError, notably because it avoids an exception in case it's not available (and doesn't catch other issues potentially).
There was a problem hiding this comment.
my thought was that the much more common case will have Coverage.running? implemented so checking for that every time would be slower than rescuing in the rarer case. but this is not a place where slowness matters at all, and I do prefer not to use exceptions for control flow, so agree with you and have updated it.
|
FYI I'm planning to try adding TruffleRuby in CI to catch such issues faster, but that needs more work so I would suggest merging this first. |
|
Confirmed this PR reduces |
|
I also started looking yesterday at adding truffle to the CI here, but got hung up on aruba/childprocess, then saw that you were ahead of me and had opened an issue about the same already. |
|
@PragTob @colszowka Hello, it would be great if this can be merged and released when you have some open-source time, because the latest release breaks a bunch of CIs testing TruffleRuby (e.g. see backlinks to this PR and #1044) |
|
Yes, it would be quite helpful if this fix could make it into a release. |
|
Ping, please merge and release this. It's been one month and a half and it still breaks the CI of e.g. https://github.com/ronin-rb/ronin/actions/runs/4089986844/jobs/7053088486 |
sferik
force-pushed
the
main
branch
9 times, most recently
from
33cbb8e to
574b45c
Compare
|
Thanks for this, sorry for all the silence. Life and more happend, been a tough year+. I'll merge, it should be fine famous simplecov maintainer last words Thanks y'all for your work 💚 @notEthan : Very cute bunny avatar! |
|
Thanks for merging. A release with this would make it usable. |
|
@notEthan If the issue is SimpleCov 0.21.2 + TruffleRuby, that's solved in recent TruffleRuby releases and since #1044 (comment), because TruffleRuby has |
4 participants
Amends 1cc7650 from #1035
Another option would be to just call Coverage.start unconditionally and rescue the RuntimeError it now raises when already started.