Coverage reports wrong number of lines #1196

Closed
donv opened this Issue Oct 31, 2013 · 11 comments

Comments

Projects
None yet
4 participants
@donv
Member

donv commented Oct 31, 2013

Given the file my_model.rb:

class MyModel
  class_eval <<-EOT, __FILE__, __LINE__
    def a
      foo
    end

    def b
      bar
    end
  EOT
end

and the following code to measure coverage in coverage_example.rb:

file = File.expand_path('my_model.rb', File.dirname(__FILE__))
puts File.read(file).split("\n").size
require 'coverage'
Coverage.start
require file.chomp('.rb')
puts Coverage.result[file].size

Run with jruby 1.7.6 and debug enabled, the output is:

$ jruby --debug coverage_example.rb 
11
15
$ 

I would expect the numbers to be the same, possibly the last to be smaller than the first.

The discrepancy trigger very noisy warning messages when measuring coverage, for example using simplecov.

What could cause the difference?

bf4 added a commit to metricfu/metric_fu that referenced this issue May 2, 2014

Turn off Coverage tests under JRuby.
 see #226
     jruby/jruby#1196
     https://jira.codehaus.org/browse/JRUBY-6106
     colszowka/simplecov#86

Travis ENV modified per
http://docs.travis-ci.com/user/ci-environment/#Environment-variables

@enebo https://twitter.com/tom_enebo/status/462289024107814914
recommended JRUBY_OPTS="-Xcli.debug=true"
but that didn't work and caused other failures
JRUBY_OPTS="--debug" Still produced the below error, as did
JRUBY_OPTS="--debug -Xcli.debug=true"

Failure message was
home/travis/build/metricfu/metric_fu/gemfiles/vendor/bundle/jruby/1.9/gems/simplecov-0.8.2/lib/simplecov.rb:31 warning: tracing (e.g. set_trace_func) will not capture all events without --debug flag

SimpleCov::Formatter::MetricFu calculates the same coverage from an RCov report as from SimpleCov
     Failure/Error: expect(source_file.coverage.count).to eq(covered_lines_from_rcov_text.count)

       expected: 11
            got: 8

       (compared using ==)
     # /home/travis/build/metricfu/metric_fu/spec/metric_fu/metrics/rcov/simplecov_formatter_spec.rb:52:in `(root)'
     # /home/travis/build/metricfu/metric_fu/spec/support/timeout.rb:6:in `(root)'

@bf4 bf4 referenced this issue in metricfu/metric_fu May 2, 2014

Merged

Fix simplecov rcov calculation #226

bf4 added a commit to metricfu/metric_fu that referenced this issue May 4, 2014

Turn off Coverage tests under JRuby.
 see #226
     jruby/jruby#1196
     https://jira.codehaus.org/browse/JRUBY-6106
     colszowka/simplecov#86

Travis ENV modified per
http://docs.travis-ci.com/user/ci-environment/#Environment-variables

@enebo https://twitter.com/tom_enebo/status/462289024107814914
recommended JRUBY_OPTS="-Xcli.debug=true"
but that didn't work and caused other failures
JRUBY_OPTS="--debug" Still produced the below error, as did
JRUBY_OPTS="--debug -Xcli.debug=true"

Failure message was
home/travis/build/metricfu/metric_fu/gemfiles/vendor/bundle/jruby/1.9/gems/simplecov-0.8.2/lib/simplecov.rb:31 warning: tracing (e.g. set_trace_func) will not capture all events without --debug flag

SimpleCov::Formatter::MetricFu calculates the same coverage from an RCov report as from SimpleCov
     Failure/Error: expect(source_file.coverage.count).to eq(covered_lines_from_rcov_text.count)

       expected: 11
            got: 8

       (compared using ==)
     # /home/travis/build/metricfu/metric_fu/spec/metric_fu/metrics/rcov/simplecov_formatter_spec.rb:52:in `(root)'
     # /home/travis/build/metricfu/metric_fu/spec/support/timeout.rb:6:in `(root)'
@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Mar 21, 2016

Member

Upon reviewing this one I can see a major issue. MRI does not store eval parses in coverage at all? So the reported issue is more bizarre than just being the wrong size. The eval happens to have the exact same name as the file so it gets its coverage data stored in the hash. The original file is left out. If I remove the FILE from the eval then JRuby has 2 entries with the original file containing the proper data and an eval entry with the data you reported. I think I just need to remove any evals from recording coverage info?

Member

enebo commented Mar 21, 2016

Upon reviewing this one I can see a major issue. MRI does not store eval parses in coverage at all? So the reported issue is more bizarre than just being the wrong size. The eval happens to have the exact same name as the file so it gets its coverage data stored in the hash. The original file is left out. If I remove the FILE from the eval then JRuby has 2 entries with the original file containing the proper data and an eval entry with the data you reported. I think I just need to remove any evals from recording coverage info?

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Mar 21, 2016

Member

Ah. So here is a clue colszowka/simplecov#38. So coverage only tracks require. I just checked and this also includes load. So I think I can just check to see if parse is eval and if so then it means it must have came from load/require.

Member

enebo commented Mar 21, 2016

Ah. So here is a clue colszowka/simplecov#38. So coverage only tracks require. I just checked and this also includes load. So I think I can just check to see if parse is eval and if so then it means it must have came from load/require.

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Mar 21, 2016

Member

Yucky wrinkle in fixing this. We enable a global hook for all LINE EVENTS whereas MRI will instrument live events into the source only for iseqs (which is IRScopes for us). Since FILE is used as the eval's name it ends up sending it's coverage events info to the coverage array and we end up mixing the eval and regular file coverage data up.

So the fix is to remove the global COVERAGE_HOOK and to replace it with instrumented code.

Member

enebo commented Mar 21, 2016

Yucky wrinkle in fixing this. We enable a global hook for all LINE EVENTS whereas MRI will instrument live events into the source only for iseqs (which is IRScopes for us). Since FILE is used as the eval's name it ends up sending it's coverage events info to the coverage array and we end up mixing the eval and regular file coverage data up.

So the fix is to remove the global COVERAGE_HOOK and to replace it with instrumented code.

@bf4

This comment has been minimized.

Show comment
Hide comment
@bf4

bf4 Mar 22, 2016

@enebo Thanks for looking into this :)

SimpleCov now has https://github.com/colszowka/simplecov/blob/cffb463890eae605a9b3f0b4437d1dbbe2271c42/lib/simplecov.rb#L5-L17

if defined?(JRUBY_VERSION) && defined?(JRuby)

  # @see https://github.com/jruby/jruby/issues/1196
  # @see https://github.com/metricfu/metric_fu/pull/226
  # @see https://github.com/colszowka/simplecov/issues/420
  # @see https://github.com/colszowka/simplecov/issues/86
  # @see https://jira.codehaus.org/browse/JRUBY-6106

  unless JRuby.runtime.debug?
    warn 'Coverage may be inaccurate; set "cli.debug=true" ("-Xcli.debug=true") in your .jrubyrc or' \
      ' do JRUBY_OPTS="-d"'
  end

and colszowka/simplecov#430

bf4 commented Mar 22, 2016

@enebo Thanks for looking into this :)

SimpleCov now has https://github.com/colszowka/simplecov/blob/cffb463890eae605a9b3f0b4437d1dbbe2271c42/lib/simplecov.rb#L5-L17

if defined?(JRUBY_VERSION) && defined?(JRuby)

  # @see https://github.com/jruby/jruby/issues/1196
  # @see https://github.com/metricfu/metric_fu/pull/226
  # @see https://github.com/colszowka/simplecov/issues/420
  # @see https://github.com/colszowka/simplecov/issues/86
  # @see https://jira.codehaus.org/browse/JRUBY-6106

  unless JRuby.runtime.debug?
    warn 'Coverage may be inaccurate; set "cli.debug=true" ("-Xcli.debug=true") in your .jrubyrc or' \
      ' do JRUBY_OPTS="-d"'
  end

and colszowka/simplecov#430

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Mar 22, 2016

Member

@bf4 I think once I address not generating events for eval code along with a local fix I made to not mutilate the coverage array locally then coverage will just be working correctly. My last comment shows it is not quite trivial but I think doable.

Member

enebo commented Mar 22, 2016

@bf4 I think once I address not generating events for eval code along with a local fix I made to not mutilate the coverage array locally then coverage will just be working correctly. My last comment shows it is not quite trivial but I think doable.

@enebo enebo closed this in 89f4c06 Mar 23, 2016

enebo added a commit that referenced this issue Mar 23, 2016

Make RootNode of AST know whether it requires coverage or not.
This is infrastructure work and not a full fix for #1196.  I
kept this separate in case we decide to fix 1.7.x as the rest
of the fixes will be in IR (which 1.7 lacks).

enebo added a commit that referenced this issue Mar 23, 2016

Fixes #1196 Coverage reports wrong number of lines...and then some.
This also fixes remaining tagged out MRI test_coverage.rb issues.
Changes:

- Event hook removes lots of extra sanity checks which almost
  entirely was fall out from evals being treated as needing
  coverage.  So, for example, random line number oddities no
  longer exist because load/require always starts at line 0.
- Removed errant printf from last commit.
- Instrument COVERAGE Events where LINE events happen so long
  as the AST/IRScope being built requires coverage.  I notice
  MRI does not emit both LINE + COVERAGE when COVERAGE is
  present so perhaps this is a future thing to change.  Adding
  this instrumentation ended up being much ickier than an-
  ticipated since we:
  a: lazily compile methods
  b: transform define_method blocks to IRMethods and re-build
  These two "lazy" builds required adding a new IRScope field
  called CODE_COVERAGE.
- Fixed inverted logic in ParserConfiguration from last commit.
- CoverageData was missing a significant piece of behavior. If
  result was called and then start was reenabled any files
  which had already had coverage happen to them are supposed to
  replace their line array with an empty one {"foo.rb" => []}.
  This was key piece to eliminating remaining MRI tests from
  failing.

I also ran specs from simplecov and it is green.  As far as I
know there are no more missing behavior from coverage.  @donv
has said he would add specs to ruby/spec for coverage.  At this
time there are no specs for coverage there at all.

@enebo enebo added this to the JRuby 9.1.0.0 milestone Mar 23, 2016

@enebo enebo added the stdlib label Mar 23, 2016

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Mar 23, 2016

Member

So this will only be fixed from JRuby 9k and not JRuby 1.7 because we have a completely different runtime and the effort to fix this on 1.7 will be more than it was for 9k. If there is enough pushback someone can open a new issue against 1.7.

Member

enebo commented Mar 23, 2016

So this will only be fixed from JRuby 9k and not JRuby 1.7 because we have a completely different runtime and the effort to fix this on 1.7 will be more than it was for 9k. If there is enough pushback someone can open a new issue against 1.7.

@PragTob

This comment has been minimized.

Show comment
Hide comment
@PragTob

PragTob Jan 25, 2017

@enebo do I understand your comment correctly to mean that JRuby 9k+ shouldn't the comment of warning? It was attached to the 9.1 milestone so should it rather not be applicable to JRuby 9.1+?

As a reference I reran the examples of @donv :

tobi@speedy ~/dev/coverage_sample $ asdf local ruby jruby-9.1.7.0
tobi@speedy ~/dev/coverage_sample $ jruby runner.rb 
11
runner.rb:4: warning: tracing (e.g. set_trace_func) will not capture all events without --debug flag
11
tobi@speedy ~/dev/coverage_sample $ jruby --debug runner.rb 
11
11
tobi@speedy ~/dev/coverage_sample $ asdf local ruby jruby-9.0.5.0
tobi@speedy ~/dev/coverage_sample $ jruby runner.rb 
11
runner.rb:4: warning: tracing (e.g. set_trace_func) will not capture all events without --debug flag
7
tobi@speedy ~/dev/coverage_sample $ jruby --debug runner.rb 
11
7
tobi@speedy ~/dev/coverage_sample $ 

9.0.5.0 looks off but off in a different manner (albeit both with --debug and without)

edit: also just discovered another failure in the simplecov test suite that happens only on 9.0.5.0 but not 9.1 - so I guess the fixes really were 9.1, thanks for taking the time Tom!

PragTob commented Jan 25, 2017

@enebo do I understand your comment correctly to mean that JRuby 9k+ shouldn't the comment of warning? It was attached to the 9.1 milestone so should it rather not be applicable to JRuby 9.1+?

As a reference I reran the examples of @donv :

tobi@speedy ~/dev/coverage_sample $ asdf local ruby jruby-9.1.7.0
tobi@speedy ~/dev/coverage_sample $ jruby runner.rb 
11
runner.rb:4: warning: tracing (e.g. set_trace_func) will not capture all events without --debug flag
11
tobi@speedy ~/dev/coverage_sample $ jruby --debug runner.rb 
11
11
tobi@speedy ~/dev/coverage_sample $ asdf local ruby jruby-9.0.5.0
tobi@speedy ~/dev/coverage_sample $ jruby runner.rb 
11
runner.rb:4: warning: tracing (e.g. set_trace_func) will not capture all events without --debug flag
7
tobi@speedy ~/dev/coverage_sample $ jruby --debug runner.rb 
11
7
tobi@speedy ~/dev/coverage_sample $ 

9.0.5.0 looks off but off in a different manner (albeit both with --debug and without)

edit: also just discovered another failure in the simplecov test suite that happens only on 9.0.5.0 but not 9.1 - so I guess the fixes really were 9.1, thanks for taking the time Tom!

PragTob added a commit to colszowka/simplecov that referenced this issue Jan 25, 2017

Officially Support JRuby 9.1+
I've personally been running simplecov with JRuby 1.7+ for years
so I'd say it is good to go either way ;P

The reality of edge cases is more complicated but it seems the
fine folks at @jruby (namely @enebo ) made the last fixes
necessary as seen in jruby/jruby#1196 so that they're available
in 9.1+ releases! The others should work, but have some little
failures (9.0.5.0 fails on one cuke for me).

So, the easiest way forward is to support 9.1+. Sadly the tests
take a while as they always start new interpreter instances and
startup time isn't exactly JRuby's strength, but CI does it
so it's bearable. :)

If anyone has problems with JRuby on a PR, in an issue or whatever
please feel free to ping me (@PragTob)

Also, special thanks goes to @donv who created did a lot of testing,
issue reporting et. al. for simplecov on JRuby for a long time!

* fixes #524

@PragTob PragTob referenced this issue in colszowka/simplecov Jan 25, 2017

Merged

Officially Support JRuby 9.1+ #547

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Jan 26, 2017

Member

@PragTob so do I need to comment on anything now? I did not quite get your original question and then I see the edit.

Member

enebo commented Jan 26, 2017

@PragTob so do I need to comment on anything now? I did not quite get your original question and then I see the edit.

@PragTob

This comment has been minimized.

Show comment
Hide comment
@PragTob

PragTob Jan 26, 2017

@enebo question was more to confirm that coverage is only expected to work fully in 9.1+ to which I found out the answer is most likely yes so that's the official support target now. Thanks! :)

PragTob commented Jan 26, 2017

@enebo question was more to confirm that coverage is only expected to work fully in 9.1+ to which I found out the answer is most likely yes so that's the official support target now. Thanks! :)

@PragTob

This comment has been minimized.

Show comment
Hide comment
@PragTob

PragTob Feb 4, 2017

@enebo I remembered my question! Right now simplecov still propmpts users to enable the debug mode, is this still necessary or should we be good without debug mode?

PragTob commented Feb 4, 2017

@enebo I remembered my question! Right now simplecov still propmpts users to enable the debug mode, is this still necessary or should we be good without debug mode?

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Feb 9, 2017

Member

@PragTob sorry for the long delay in replying but I was travelling. We still need --debug because we do not emit trace instructions into our emitted IR instructions without it. In the future we can invalidate ALL methods once tracing is called to re-compile all methods to emit a version with these instrs but it is not high in the priority list...

Member

enebo commented Feb 9, 2017

@PragTob sorry for the long delay in replying but I was travelling. We still need --debug because we do not emit trace instructions into our emitted IR instructions without it. In the future we can invalidate ALL methods once tracing is called to re-compile all methods to emit a version with these instrs but it is not high in the priority list...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment