Update Coverage for Ruby 3.2 interface changes.

[ioquatix]

Fixes #3149.
Related #1636.

Tests for this should be in ruby spec itself.

[eregon]

Tests for this should be in ruby spec itself.

You mean there are already specs for this, right? Like https://github.com/ruby/spec/blob/master/library/coverage/supported_spec.rb
If so we should untag them, via jt untag spec/ruby/library/coverage/supported_spec.rb (after jt build)

[ioquatix]

Do you mind doing this untag process? I am in Japan with only my laptop and apparently arm64 is not a supported architecture :)

[eregon]

This doesn't seem correct, e.g. if lines: false we shouldn't enable coverage.

For unknown kwargs, should we raise ArgumentError?

Surprisingly CRuby seems to ignores extra kwargs:

$ ruby -v -rcoverage -e 'Coverage.start(foo: true)'   
ruby 3.2.2 (2023-03-30 revision e51014f9c0) [x86_64-linux]

But it does not allow any positional argument:

$ ruby -v -rcoverage -e 'Coverage.start(1)'        
ruby 3.2.2 (2023-03-30 revision e51014f9c0) [x86_64-linux]
-e:1:in `start': no implicit conversion of Integer into Hash (TypeError)

Coverage.start(1)
               ^
	from -e:1:in `<main>'

$ ruby -v -rcoverage -e 'Coverage.start(1, foo: true)'
ruby 3.2.2 (2023-03-30 revision e51014f9c0) [x86_64-linux]
-e:1:in `start': wrong number of arguments (given 2, expected 0..1) (ArgumentError)

Coverage.start(1, foo: true)
               ^^^^^^^^^^^^
	from -e:1:in `<main>'

[eregon]

This doesn't seem correct, e.g. if lines: false we shouldn't enable coverage.

Actually that seems the CRuby semantics :/
So that part seems OK.

$ ruby -v -rcoverage -e 'Coverage.start(foo: true); p Coverage.running?'
ruby 3.2.2 (2023-03-30 revision e51014f9c0) [x86_64-linux]
true

(and also ruby -v -rcoverage -e 'Coverage.start(foo: true); p Coverage.running?; require "drb"; p Coverage.result')
Even for lines: false it enables it.

[ioquatix]

Maybe we should write more ruby specs for this behaviour? I think we can tighten it up for 3.3.

[eregon]

Yes that would be great, and you can add the specs in this PR and they'll get sync'd in ruby/spec.

I think we can tighten it up for 3.3.

I'm not sure we can change this confusing behavior, compatibility-wise. At least it seems useful to spec the current behavior, even though it's confusing.
Maybe it's designed so Coverage.start never raises an error or so (when there wasn't Coverage.supported?).

[eregon]

Do you mind doing this untag process? I am in Japan with only my laptop and apparently arm64 is not a supported architecture :)

It is supported (both on linux and macos).
But sure I can do the jt untag

[mtortonesi]

Are you sure there is nothing wrong with the result method too?

I applied your patch to truffleruby+graalvm-23.0.0 (installed from rbenv) and in my code using sus+covered I keep getting:

 core/type.rb:276:in `convert_type': no implicit conversion of Symbol into Integer (TypeError)
	from  core/type.rb:178:in `rb_to_int_fallback'
	from /Users/mauro/.rbenv/versions/truffleruby+graalvm-23.0.0/graalvm/Contents/Home/languages/ruby/lib/gems/gems/covered-0.21.0/lib/covered/capture.rb:40:in `[]'

Apparently, result returns a Hash of this type:

{ "/Users/mauro/.rbenv/versions/truffleruby+graalvm-23.0.0/graalvm/Contents/Home/languages/ruby/lib/gems/gems/sus-0.21.1/lib/sus/tree.rb"=>[1, 1, 1, 0, nil, nil, 1, 0, nil, 0, nil, 0, 0, 0, nil, nil, nil, 0, nil, nil, 1, 0, 0, nil, nil, nil, nil], ...

but the covered code expects a hash with a :lines key instead of an array.

[eregon]

@mtortonesi Interesting, that seems a difference between Coverage.start(lines: true) and Coverage.start, affecting Coverage.result.

$ ruby -v -rcoverage -e 'Coverage.start(lines: true); require "benchmark"; p Coverage.result'                   
ruby 3.1.3p185 (2022-11-24 revision 1a6b16756e) [x86_64-linux]
{"/home/eregon/.rubies/ruby-3.1.3/lib/ruby/3.1.0/benchmark.rb"=>{:lines=>[nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, 1, nil, 1, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, nil, 0, nil, 0, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, 1, 0, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, 1, 0, 0, 0, 0, 0, nil, nil, 0, 0, 0, 0, 0, 0, nil, 0, nil, nil, 0, 0, 0, 0, 0, nil, nil, 0, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, 1, 0, 0, 0, 0, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, 1, 0, 0, 0, nil, nil, 1, nil, nil, nil, nil, nil, 1, nil, nil, nil, nil, nil, nil, nil, 1, 0, 0, nil, nil, nil, nil, nil, 1, 0, 0, 0, 0, 0, 0, nil, nil, 1, nil, nil, 1, nil, nil, 1, nil, nil, nil, nil, nil, nil, 1, nil, nil, nil, nil, nil, nil, nil, 1, 0, nil, nil, nil, nil, nil, nil, nil, 1, 0, 0, 0, 0, nil, nil, 1, nil, nil, 1, nil, nil, nil, nil, nil, nil, nil, nil, 1, nil, nil, 1, nil, nil, 1, nil, nil, 1, nil, nil, 1, nil, nil, 1, nil, nil, 1, nil, nil, 1, nil, nil, 1, nil, nil, 1, nil, nil, nil, nil, nil, nil, nil, 1, 0, 0, nil, nil, nil, nil, nil, nil, 1, 0, nil, nil, nil, nil, nil, nil, nil, nil, 1, 0, 0, 0, 0, 0, 0, 0, nil, nil, nil, nil, nil, nil, nil, nil, 1, nil, nil, nil, nil, nil, nil, 1, nil, nil, nil, nil, nil, 1, nil, nil, nil, nil, nil, nil, 1, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, nil, nil, nil, nil, nil, 1, 0, nil, nil, nil, nil, nil, nil, nil, nil, 1, 0, nil, nil, nil, nil, nil, 1, nil, 0, nil, nil, nil, nil, nil, nil, nil, nil, 1, nil, nil, nil, nil, nil, nil, nil, nil, nil, 1, 0, nil, 0, nil, nil, nil, nil, nil, nil, 0, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, 1, nil, nil, 1, nil]}}

$ ruby -v -rcoverage -e 'Coverage.start(); require "benchmark"; p Coverage.result'    
ruby 3.1.3p185 (2022-11-24 revision 1a6b16756e) [x86_64-linux]
{"/home/eregon/.rubies/ruby-3.1.3/lib/ruby/3.1.0/benchmark.rb"=>[nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, 1, nil, 1, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, nil, 0, nil, 0, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, 1, 0, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, 1, 0, 0, 0, 0, 0, nil, nil, 0, 0, 0, 0, 0, 0, nil, 0, nil, nil, 0, 0, 0, 0, 0, nil, nil, 0, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, 1, 0, 0, 0, 0, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, 1, 0, 0, 0, nil, nil, 1, nil, nil, nil, nil, nil, 1, nil, nil, nil, nil, nil, nil, nil, 1, 0, 0, nil, nil, nil, nil, nil, 1, 0, 0, 0, 0, 0, 0, nil, nil, 1, nil, nil, 1, nil, nil, 1, nil, nil, nil, nil, nil, nil, 1, nil, nil, nil, nil, nil, nil, nil, 1, 0, nil, nil, nil, nil, nil, nil, nil, 1, 0, 0, 0, 0, nil, nil, 1, nil, nil, 1, nil, nil, nil, nil, nil, nil, nil, nil, 1, nil, nil, 1, nil, nil, 1, nil, nil, 1, nil, nil, 1, nil, nil, 1, nil, nil, 1, nil, nil, 1, nil, nil, 1, nil, nil, 1, nil, nil, nil, nil, nil, nil, nil, 1, 0, 0, nil, nil, nil, nil, nil, nil, 1, 0, nil, nil, nil, nil, nil, nil, nil, nil, 1, 0, 0, 0, 0, 0, 0, 0, nil, nil, nil, nil, nil, nil, nil, nil, 1, nil, nil, nil, nil, nil, nil, 1, nil, nil, nil, nil, nil, 1, nil, nil, nil, nil, nil, nil, 1, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, nil, nil, nil, nil, nil, 1, 0, nil, nil, nil, nil, nil, nil, nil, nil, 1, 0, nil, nil, nil, nil, nil, 1, nil, 0, nil, nil, nil, nil, nil, nil, nil, nil, 1, nil, nil, nil, nil, nil, nil, nil, nil, nil, 1, 0, nil, 0, nil, nil, nil, nil, nil, nil, 0, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, 1, nil, nil, 1, nil]}

[mtortonesi]

@eregon wow. You really weren't kidding about Coverage having a "confusing behavior"...

What is the best way to deal with that? Adding to TruffleRuby's Coverage class a class variable keeping track if the start method was called with a :lines argument?

[eregon]

Yeah Ruby's Coverage feels very hacky. Maybe we should expose the Truffle Coverage tool better instead.
OTOH there is of course value in being as compatible as possible with CRuby for the Coverage module too.

Right, that could be tracked as an instance variable on the Coverage module.

[mtortonesi]

Can confirm this code works for me:

  def self.start(*arguments, **options)
    # Arguments/options are ignored
    @lines = true if options[:lines]
    Truffle::Coverage.enable
  end

  def self.result
    result = peek_result
    Truffle::Coverage.disable
    result.transform_values! { |_,lines_array| {lines: lines_array} } if @lines
    result
  end

AFAIK, transform_values! is Ruby 2.4+ only, though.

[eregon]

@ioquatix Could you integrate that in your PR and add specs?
Or if you don't really have time, maybe @mtortonesi would be interested to make a PR fixing all we mentioned here?

[mtortonesi]

As per @eregon's suggestion, I created a new pull request (#3151) to fix this issue.

[ioquatix]

I'll close this in favour of #3151