-
Notifications
You must be signed in to change notification settings - Fork 185
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
Update Coverage for Ruby 3.2 interface changes. #3151
Conversation
Thank you for your pull request and welcome to our community! To contribute, please sign the Oracle Contributor Agreement (OCA).
To sign the OCA, please create an Oracle account and sign the OCA in Oracle's Contributor Agreement Application. When signing the OCA, please provide your GitHub username. After signing the OCA and getting an OCA approval from Oracle, this PR will be automatically updated. If you are an Oracle employee, please make sure that you are a member of the main Oracle GitHub organization, and your membership in this organization is public. |
lib/truffle/coverage.rb
Outdated
def self.start(*arguments, **options) | ||
# We have to track if the :lines option was provided, as that calls for a | ||
# different result format | ||
@lines = true if options[:lines] |
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.
@lines = true if options[:lines] | |
@lines = !!options[:lines] |
You will want to set or clear this every time to ensure that the state is handled correctly.
For ::Coverage.result(stop: false, clear: true) |
Thank you for signing the OCA. |
Dear @ioquatix, I changed the code as per your request. Would be happy to help you with adding support for ::Coverage.result(stop: false, clear: true) but I need more information about it. |
def self.result(stop: true, clear: true)
result = peek_result
Truffle::Coverage.disable if stop || clear
Truffle::Coverage.enable if !stop && clear I believe something like this can work.. but it's just a guess. |
I took another look at this and fixed a couple of bugs in my code. Now everything seems to be fine: Example video of sus+covered on Truffleruby @ioquatix, I also added support for the stop and clear argument in Coverage#result, following your suggestions. However, I have no idea how to test that code properly. |
Thank you for this this. |
Having these specs is needed to ensure we implement working and compatible behavior + avoid regressions. |
I will add missing specs and prepare the PR to merging. |
89d5c41
to
44424e8
Compare
@mtortonesi Sorry it takes a while but as you can see it's a lot more complicated than the initial patch. We'll merge the changes when they are ready, we have an internal PR with extra specs etc. |
Fixed in #3389 |
This patch request should fix issue #3149.
Based on #3150
Thanks to @ioquatix and @eregon.