Bug #16776
openRegression in coverage library
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) over 4 years 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) over 4 years 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) over 4 years 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) over 4 years 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) over 4 years 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) over 4 years 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) over 4 years 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) over 4 years 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 usingdeep-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!
Updated by hsbt (Hiroshi SHIBATA) almost 3 years ago
- Status changed from Open to Assigned
- Assignee set to mame (Yusuke Endoh)
Updated by mame (Yusuke Endoh) almost 3 years ago
- Assignee changed from mame (Yusuke Endoh) to ko1 (Koichi Sasada)
Updated by mame (Yusuke Endoh) almost 3 years ago
- Related to Feature #15912: Allow some reentrancy during TracePoint events added