Project

General

Profile

Bug #16776

Regression in coverage library

Added by deivid (David Rodríguez) 3 months ago. Updated 3 months ago.

Status:
Open
Priority:
Normal
Assignee:
-
Target version:
-
[ruby-core:97810]

Description

Hi!

I noticed a regression in the coverage library. I tried to write a minimal program to show it, hopefully it gives some clues or where the issue might lie.

In ruby 2.5.8 and earlier, the following program would print {:lines=>[1, 1, nil]}, showing that the body of the "foo" method was run once. However, on newer rubies, it prints {:lines=>[1, 0, nil]}, which is incorrect because the "foo" method body has actually been run once.

This is the repro script:

# frozen_string_literal: true

require "coverage"
Coverage.start(lines: true)

code = <<~RUBY
  def foo
    "LOL"
  end
RUBY

File.open("foo.rb", "w") { |f| f.write(code) }

require_relative "foo"

TracePoint.new(:line) do |_tp|
  foo
end.enable do
  sleep 0
end

res = Coverage.result
puts res[File.expand_path("foo.rb")]

Updated by mame (Yusuke Endoh) 3 months ago

Thank you for your report. The reason why the method execution is missed is because:

  • coverage measurement is based on TracePoint mechanism (since 2.6), and
  • no TracePoint event is fired during the execution of TracePoint hooks (to avoid infinite chain of hooks).

So, this is a limitation of TracePoint. Currently, the combination of multiple TracePoint hooks works not so great; they are completely exclusive.

This issue may be solved by improving TracePoint mechanism, but it is expensive to implement and ko1 (Koichi Sasada) is negative. How severe is this issue for you?

Updated by deivid (David Rodríguez) 3 months ago

I see.

It's a severe as "I can no longer track code coverage on my library that makes heavy use of TracePoint" is considered.

(Edited the message to add thank you for your response and your work in general :))

Updated by marcandre (Marc-Andre Lafortune) 3 months ago

I never realized this limitation. I imagine that for the vast majority of users it's a non issue, but for byebug it is quite serious. Would you consider using deep-cover instead?

Updated by marcandre (Marc-Andre Lafortune) 3 months ago

I'm not super familiar with TracePoint, but at first sight DeepCover shouldn't generate any extra callbacks as the instrumenting is minimal, typically adding $tracker[42] += 1 and taking care to never change the line numbers.

So it should work even with TracePoint enabled and shouldn't impact your tests. I tested quickly with DeepCover's clone mode and confirm that your test suite appear to pass :-)

Updated by Eregon (Benoit Daloze) 3 months ago

I'm surprised too the coverage stdlib gets hit by this issue.

Maybe coverage could use a second type of internal tracepoint to fix this issue (internal coverage handlers are likely pure C and so don't emit any extra event)?

Updated by mame (Yusuke Endoh) 3 months ago

IMO, TracePoint hooks should have a priority. A TracePoint hook that is first enabled should monitor a hook code that is second enabled:

1: TracePoint.new(:line) do |tp|
2:   p [:parent, tp]
3: end.enable do
4:   TracePoint.new(:line) do |tp|
5:     p [:child, tp]
6:   end.enable do
7:     p :main
8:   end
9: end
$ ruby t.rb
[:parent, #<TracePoint:line@t.rb:4>]
[:parent, #<TracePoint:line@<internal:trace_point>:96 in `new'>]
[:parent, #<TracePoint:line@<internal:trace_point>:196 in `enable'>]
[:child, #<TracePoint:line@t.rb:7>]
[:parent, #<TracePoint:line@t.rb:7>]
:main

I expect [:parent, #<TracePoint:line@t.rb:5>] before [:child, #<TracePoint:line@t.rb:7>]. ko1 (Koichi Sasada), what do you think?

Updated by mame (Yusuke Endoh) 3 months ago

Eregon (Benoit Daloze) wrote in #note-5:

Maybe coverage could use a second type of internal tracepoint to fix this issue (internal coverage handlers are likely pure C and so don't emit any extra event)?

Yes, it is the plan B that I have. I don't like it as it is very ad-hoc, though. If ko1 (Koichi Sasada) rejects the priority, I'll give it a try to implement.

Updated by deivid (David Rodríguez) 3 months ago

marcandre (Marc-Andre Lafortune) wrote in #note-3:

I never realized this limitation. I imagine that for the vast majority of users it's a non issue, but for byebug it is quite serious. Would you consider using deep-cover instead?

It sounds like the plan is to fix this one way or another, so I'll wait for now. But thanks for pointing me to deep-cover, it sounds interesting!

Also available in: Atom PDF