Bug #9572

Restarting Coverage does not produce correct coverage result

Added by Sean Ferguson over 1 year ago. Updated over 1 year ago.

ruby -v:uby 2.0.0p353 (2013-11-22 revision 43784) [x86_64-darwin12.4.0] Backport:1.9.3: UNKNOWN, 2.0.0: UNKNOWN, 2.1: UNKNOWN


Feature 4796(https://www.ruby-forum.com/topic/1811306#1001864) appears
to have a bug in it.

While the test listed there does pass the following test does not:

require "test/unit"
require "coverage"
require 'tmpdir'
class TestCoverage < Test::Unit::TestCase
def test_restarting_coverage
Dir.mktmpdir {|tmp|
Dir.chdir(tmp) {
File.open("test.rb", "w") do |f|
f.puts <<-EOS
def coverage_test_method
puts :ok
require tmp + '/test.rb'
result = Coverage.result
assert_equal 1, result.size
assert_equal [0, 1, nil], result.first[1] # coverage stats show an empty array here

It appears that while the coverage is finding the correct files it is
not giving any coverage stats for those files. Knowing this information would be very helpful in determining test coverage data for individual test files. I'm not very familiar
with how the coverage library works, but if you can point me at where to
look I can give fixing it a try.


Sean Ferguson


#1 Updated by Sam Rawlins over 1 year ago

Good catch. My guess is that #4796 is not implemented in the way you are hoping for, and cannot easily be implemented in this way either.

The original test for #4796 (contributed by xavier-shay in #4796), before r33030 [1], only asserted that the file still existed in the Coverage.result hash, not that the hash value for that file key was correct. r33030 changed the test to in fact assert that the hash value is an empty something (#size == 0).

So #4796 allows Coverage to be restartable, but with the following caveats:
* Only new files that are required after Coverage is restarted (and all files that are loaded after Coverage is restarted) will be properly traced.
* Files that were required before Coverage is restarted will exist in the Coverage.result hash, but with empty hash values.

This can be changed so that files required before Coverage is restarted will have inaccurate coverage (rather than empty coverage). I think you acknowledge this in your amended test, where you assert that the coverage report for the test file should be [0,1,nil]. That is, the def coverage_test_method line is not covered, because it was parsed earlier, before Coverage was restarted, but the puts :ok line is still covered, because the method is called after Coverage is restarted.

I'm not really in favor of knowingly reporting inaccurate data on restart, but you could get this functionality (and pass your test) with the following patch:

diff --git a/ext/coverage/coverage.c b/ext/coverage/coverage.c
index 93cb2a5..770d81c 100644
--- a/ext/coverage/coverage.c
+++ b/ext/coverage/coverage.c
@@ -38,8 +38,14 @@ coverage_result_i(st_data_t key, st_data_t val, st_data_t h)
     VALUE path = (VALUE)key;
     VALUE coverage = (VALUE)val;
     VALUE coverages = (VALUE)h;
+    long i;
     coverage = rb_ary_dup(coverage);
-    rb_ary_clear((VALUE)val);
+    for (i=0; i<RARRAY_LEN(val); i++) {
+       if (RARRAY_AREF(val, i) != Qnil)
+           RARRAY_ASET(val, i, INT2FIX(0));
+    }
     rb_hash_aset(coverages, path, coverage);
     return ST_CONTINUE;

[1] http://svn.ruby-lang.org/cgi-bin/viewvc.cgi/trunk/test/coverage/test_coverage.rb?r1=32404&r2=33030&diff_format=h

#2 Updated by Dan Mayer over 1 year ago

I have been having this problem as well.

It seems good that Coverage.result resets the coverage data, but once Coverage.start is called again it seems like it should start building up new coverage from that point on. In the example code I have for instance I would expect the second call to coverage results, to not show method_a being called, but it should show method_b.

Check this small git repo for an example of the problem I am seeing. https://github.com/danmayer/coverage-bug

Without being able to collect additional coverage data after restarting it makes gathering coverage data via sampling not possible. Perhaps this is the expected output, if so could someone explain expected behavior or why the current way would be preferred.

Also available in: Atom PDF