Feature #4796

Coverage should be restartable

Added by Xavier Shay over 4 years ago. Updated about 4 years ago.

[ruby-core:36539]
Status:Closed
Priority:Normal
Assignee:Tomoyuki Chikanaga

Description

I would like a way to be able to make the following test past:

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
            end
          EOS
        end

        Coverage.start
        require tmp + '/test.rb'
        Coverage.result
        Coverage.start
        coverage_test_method
        assert_equal 1, Coverage.result.size
      }
    }
  end
end

The problem is that Coverage.start doesn't track any files loaded before it is called. This is probably desired behaviour so that stdlib files are not tracked, but it limits the usefulness of Coverage. Specifically, I am trying to collate coverage reports from workers in multiple processes. Also I want to associate coverages with specific tests (this test executed this code, etc...). This is very difficult without being able to restart.

What would be involved in doing this? If you point me in the right direction I can perhaps have a go.

Associated revisions

Revision 32404
Added by Tomoyuki Chikanaga about 4 years ago

  • ext/coverage/coverage.c: resurrect r32071 + add GC guard for
    rb_coverages. [Bug #4927]
    [Feature #4796]

  • test/coverage/test_coverage.rb resurrect r32071.

Revision 32404
Added by Tomoyuki Chikanaga about 4 years ago

  • ext/coverage/coverage.c: resurrect r32071 + add GC guard for
    rb_coverages. [Bug #4927]
    [Feature #4796]

  • test/coverage/test_coverage.rb resurrect r32071.

History

#1 Updated by Yusuke Endoh over 4 years ago

  • Status changed from Open to Assigned
  • Category set to lib
  • Assignee set to Yusuke Endoh
  • Target version set to 1.9.3

Hello.

As a maintainer of ext/coverage, I like to approve this feature unless
there is objection. But I'd like to confirm some points.

2011/5/29 Xavier Shay xavier-list@rhnh.net:

The problem is that Coverage.start doesn't track any files loaded before it is called. This is probably desired behaviour so that stdlib files are not tracked, but it limits the usefulness of Coverage.

First of all, it is difficult to track files loaded before start' is
first called. This is because the VM compiler inserts instructions to
measure coverage after
start' is first called. Since the mechanism
have considerable overhead, it cannot be enabled by default.

You are talking about second (or later) calls to Coverage.start, right?

Specifically, I am trying to collate coverage reports from workers in multiple processes.

I cannot understand why the feature is needed for this use case.
Could you elaborate this?

Also I want to associate coverages with specific tests (this test executed this code, etc...). This is very difficult without being able to restart.

I can understand this.

Here is a patch.

diff --git a/ext/coverage/coverage.c b/ext/coverage/coverage.c
index 29ac709..3a26aaa 100644
--- a/ext/coverage/coverage.c
+++ b/ext/coverage/coverage.c
@@ -11,6 +11,8 @@
#include "ruby.h"
#include "vm_core.h"

+static VALUE rb_coverages = Qundef;
+
/*
* call-seq:
* Coverage.start => nil
@@ -21,19 +23,25 @@ static VALUE
rb_coverage_start(VALUE klass)
{
if (!RTEST(rb_get_coverages())) {
- VALUE coverages = rb_hash_new();
- RBASIC(coverages)->klass = 0;
- rb_set_coverages(coverages);
+ if (rb_coverages == Qundef) {
+ rb_coverages = rb_hash_new();
+ RBASIC(rb_coverages)->klass = 0;
+ }
+ rb_set_coverages(rb_coverages);
}
return Qnil;
}

static int
-coverage_result_i(st_data_t key, st_data_t val, st_data_t dummy)
+coverage_result_i(st_data_t key, st_data_t val, st_data_t h)
{
+ VALUE path = (VALUE)key;
VALUE coverage = (VALUE)val;
- RBASIC(coverage)->klass = rb_cArray;
+ VALUE coverages = (VALUE)h;
+ coverage = rb_ary_dup(coverage);
+ rb_ary_clear((VALUE)val);
rb_ary_freeze(coverage);
+ rb_hash_aset(coverages, path, coverage);
return ST_CONTINUE;
}

@@ -48,14 +56,14 @@ static VALUE
rb_coverage_result(VALUE klass)
{
VALUE coverages = rb_get_coverages();
+ VALUE ncoverages = rb_hash_new();
if (!RTEST(coverages)) {
rb_raise(rb_eRuntimeError, "coverage measurement is not enabled");
}
- RBASIC(coverages)->klass = rb_cHash;
- st_foreach(RHASH_TBL(coverages), coverage_result_i, 0);
- rb_hash_freeze(coverages);
+ st_foreach(RHASH_TBL(coverages), coverage_result_i, ncoverages);
+ rb_hash_freeze(ncoverages);
rb_reset_coverages();
- return coverages;
+ return ncoverages;
}

/* Coverage provides coverage measurement feature for Ruby.

Yusuke Endoh mame@tsg.ne.jp

#2 Updated by Xavier Shay over 4 years ago

You are talking about second (or later) calls to Coverage.start, right?
Yes, that would be fine.

I cannot understand why the feature is needed for this use case.
Could you elaborate this?

It's not required, it just would of made my life easier. I was trying to integrate with the parallel and SimpleCov gems, and ended up with a work around which involves conditionally hooking at_exit and a file lock, which is a bit of a mess.

Here is a patch.
I will try it out and report back. Thanks!

#3 Updated by Xavier Shay about 4 years ago

I tried this patch and it works as expected. Here is a patch for the test case, slightly cleaned up:

diff --git a/test/coverage/test_coverage.rb b/test/coverage/test_coverage.rb
index ace49d3..56966b1 100644
--- a/test/coverage/test_coverage.rb
+++ b/test/coverage/test_coverage.rb
@@ -1,5 +1,6 @@
require "test/unit"
require "coverage"
+require "tmpdir"

class TestCoverage < Test::Unit::TestCase
def test_result_without_start
@@ -14,4 +15,29 @@ class TestCoverage < Test::Unit::TestCase
assert_kind_of(Array, val)
end
end
+
+ def test_restarting_coverage
+ loaded_features = $".dup
+
+ Dir.mktmpdir {|tmp|
+ Dir.chdir(tmp) {
+ File.open("test.rb", "w") do |f|
+ f.puts <<-EOS
+ def coverage_test_method
+ :ok
+ end
+ EOS
+ end
+
+ Coverage.start
+ require tmp + '/test.rb'
+ Coverage.result
+ Coverage.start
+ coverage_test_method
+ assert_equal 1, Coverage.result.size
+ }
+ }
+ ensure
+ $".replace loaded_features
+ end
end

#4 Updated by Yusuke Endoh about 4 years ago

  • Status changed from Assigned to Closed

Sorry for late action. I've committed my implementation and
your test patch. Thank you!

Yusuke Endoh mame@tsg.ne.jp

#5 Updated by Yusuke Endoh about 4 years ago

  • Status changed from Closed to Open
  • Priority changed from Normal to 3
  • Target version changed from 1.9.3 to 2.0.0
  • Assignee deleted (Yusuke Endoh)

Unfortunately this patch seems to cause SEGV on os x. [#4927]
I cannot fix it because I have no os x. So I can't help but
revert it.

Anyone who wants this feature, please fix it by yourself or
please give me a Mac :-)

Yusuke Endoh mame@tsg.ne.jp

#6 Updated by Tomoyuki Chikanaga about 4 years ago

  • Assignee set to Tomoyuki Chikanaga
  • Status changed from Open to Assigned
  • Target version changed from 2.0.0 to 1.9.3

Sorry, I don't have extra Mac. But I can show a patch.
I'll commit it.

#7 Updated by Tomoyuki Chikanaga about 4 years ago

  • Status changed from Assigned to Closed
  • % Done changed from 0 to 100

This issue was solved with changeset r32404.
Xavier, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


  • ext/coverage/coverage.c: resurrect r32071 + add GC guard for
    rb_coverages. [Bug #4927]
    [Feature #4796]

  • test/coverage/test_coverage.rb resurrect r32071.

#8 Updated by Yusuke Endoh about 4 years ago

That's the best! Thank you nagachika!

Yusuke Endoh mame@tsg.ne.jp

Also available in: Atom PDF