Wrong number of reported lines in Coverage API #1981

Closed
donv opened this Issue Sep 17, 2014 · 15 comments

Comments

Projects
None yet
4 participants
@donv
Member

donv commented Sep 17, 2014

JRuby does not report commented and blank lines in the coverage api. This leads to noisy and unreliable coverage reports.

Code used to reproduce:

require 'coverage'
file_name = File.expand_path('dummy.rb')
File.write(file_name, "# Comment\n\n")
puts "Line count: #{File.readlines(file_name).size}"
Coverage.start
require file_name.chomp('.rb')
result = Coverage.result
puts "Coverage: #{result}"
puts "Coverage: #{result[file_name].size}"

MRI reports 2 lines, and JRuby fails to report the file at all. JRuby should report 2 lines in the given file.

$ rvm use ruby
Using ~/.rvm/gems/ruby-2.1.2
$ ruby lines.rb 
Line count: 2
Coverage: {"/Users/uwe/workspace/TelenorGS/TelenorGS/dummy.rb"=>[nil, nil]}
Coverage: 2
$ rvm use jruby
Using /Users/uwe/.rvm/gems/jruby-1.7.15
$ ruby lines.rb 
Line count: 2
lines.rb:5 warning: tracing (e.g. set_trace_func) will not capture all events without --debug flag
Coverage: {}
NoMethodError: undefined method `size' for nil:NilClass
  (root) at lines.rb:9

Related issues:

@donv donv added this to the JRuby 1.7.16 milestone Sep 17, 2014

@enebo enebo modified the milestones: JRuby 1.7.17, JRuby 1.7.16 Sep 24, 2014

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Sep 24, 2014

Member

1.7.16 is out this week and I have no time to look at this. @donv since you elevated this I just bumped it to 1.7.17, but I don't know when I can look at this.

Member

enebo commented Sep 24, 2014

1.7.16 is out this week and I have no time to look at this. @donv since you elevated this I just bumped it to 1.7.17, but I don't know when I can look at this.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Dec 2, 2014

Member

This is a nontrivial thing to fix. It would require us to modify the interpreter (and maybe the parser) to emit events for every newline. Currently I believe many (most? all?) newlines that have no actual code get collapsed into single NewlineNode instances. I'll take a look at the AST and see how bad it is.

Member

headius commented Dec 2, 2014

This is a nontrivial thing to fix. It would require us to modify the interpreter (and maybe the parser) to emit events for every newline. Currently I believe many (most? all?) newlines that have no actual code get collapsed into single NewlineNode instances. I'll take a look at the AST and see how bad it is.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Dec 2, 2014

Member

Yeah, it's as I feared. Source lines that don't have any actual code on them are dropped by the parser.

~/projects/jruby-1.7 $ cat blah.rb
p 1
# comment

p 2

~/projects/jruby-1.7 $ ast blah.rb 
AST:
RootNode 0
  BlockNode 0
    NewlineNode 0
      FCallOneArgNode:p 0
        ArrayNode 0
          FixnumNode 0
, null
    NewlineNode 3
      FCallOneArgNode:p 3
        ArrayNode 3
          FixnumNode 3
, null
Member

headius commented Dec 2, 2014

Yeah, it's as I feared. Source lines that don't have any actual code on them are dropped by the parser.

~/projects/jruby-1.7 $ cat blah.rb
p 1
# comment

p 2

~/projects/jruby-1.7 $ ast blah.rb 
AST:
RootNode 0
  BlockNode 0
    NewlineNode 0
      FCallOneArgNode:p 0
        ArrayNode 0
          FixnumNode 0
, null
    NewlineNode 3
      FCallOneArgNode:p 3
        ArrayNode 3
          FixnumNode 3
, null
@donv

This comment has been minimized.

Show comment
Hide comment
@donv

donv Dec 3, 2014

Member

What happens if they are not dropped?

Member

donv commented Dec 3, 2014

What happens if they are not dropped?

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Dec 3, 2014

Member

Everything runs slower (AST interp needs to traverse every node to execute)...

...but I have been thinking about this and there are two possible solutions (both which can be done at same time):

  1. Only strip out contiguous newline nodes if they have the same line number

This will also be slower since we will be interp'ing multiple extra newline nodes but it will be a mix. I might try this one first to look for impact then move on to two.

  1. Do not strip out newline nodes (or only strip out line in #1) if --profile option is specified.

I am not sure but don't we need an option like --profile for stuff to work? I will look at option 1 and if I cannot see much change in interping these extra newlines we might go with that.

Member

enebo commented Dec 3, 2014

Everything runs slower (AST interp needs to traverse every node to execute)...

...but I have been thinking about this and there are two possible solutions (both which can be done at same time):

  1. Only strip out contiguous newline nodes if they have the same line number

This will also be slower since we will be interp'ing multiple extra newline nodes but it will be a mix. I might try this one first to look for impact then move on to two.

  1. Do not strip out newline nodes (or only strip out line in #1) if --profile option is specified.

I am not sure but don't we need an option like --profile for stuff to work? I will look at option 1 and if I cannot see much change in interping these extra newlines we might go with that.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Dec 4, 2014

Member

I like option 1. :-)

I think you mean --debug for full trace events to work. I believe that is necessary to get accurate coverage numbers, because the JIT doesn't emit all the line number events. It probably will in 9k though.

Member

headius commented Dec 4, 2014

I like option 1. :-)

I think you mean --debug for full trace events to work. I believe that is necessary to get accurate coverage numbers, because the JIT doesn't emit all the line number events. It probably will in 9k though.

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Dec 4, 2014

Member

ok there is a larger problem and this bug is not so simple to fix. I am adding some notes for the next attempt (perhaps we may only do this for 9k too).

In RubyYaccLexer. I can add comments and newlines which never are destined to be real lines to be marked as needing coverage data. For newline_node in ParserSupport I can add logic to make sure I don't strip out a relevent newlinenode. There are two other places where newlines are important for 1.7 but since they are not stripped out I can also add those elements to mark themselves important for coverage. So marking is simple.

whitespace-only lines and comment lines never actually emit anything to the parser. The lexer does not think they are important enough to add them to the AST. Having implemented a really really janky system for saving non-essential syntax in the jruby-parser project I am going to say we will never add that system to jruby proper. So what to do?

I have not seen how mri copes with this so perhaps they do something which will give an aha moment.
In 9k, I think if debug is on I can emit raw missing linenumber instrs during IRBuild with the scope boundaries helping to emit coverage properly. So I will add to 9k and probably retarget back to 1.7 if reading MRI I learn a simple way of doing this (after all MRI does not emit comments and blank lines either -- of course mri1.9 is instr-based like 9k).

Member

enebo commented Dec 4, 2014

ok there is a larger problem and this bug is not so simple to fix. I am adding some notes for the next attempt (perhaps we may only do this for 9k too).

In RubyYaccLexer. I can add comments and newlines which never are destined to be real lines to be marked as needing coverage data. For newline_node in ParserSupport I can add logic to make sure I don't strip out a relevent newlinenode. There are two other places where newlines are important for 1.7 but since they are not stripped out I can also add those elements to mark themselves important for coverage. So marking is simple.

whitespace-only lines and comment lines never actually emit anything to the parser. The lexer does not think they are important enough to add them to the AST. Having implemented a really really janky system for saving non-essential syntax in the jruby-parser project I am going to say we will never add that system to jruby proper. So what to do?

I have not seen how mri copes with this so perhaps they do something which will give an aha moment.
In 9k, I think if debug is on I can emit raw missing linenumber instrs during IRBuild with the scope boundaries helping to emit coverage properly. So I will add to 9k and probably retarget back to 1.7 if reading MRI I learn a simple way of doing this (after all MRI does not emit comments and blank lines either -- of course mri1.9 is instr-based like 9k).

@enebo enebo modified the milestones: JRuby 9.0.0.0, JRuby 1.7.17 Dec 4, 2014

@donv

This comment has been minimized.

Show comment
Hide comment
@donv

donv Dec 18, 2014

Member

Hi @enebo ! Did you learn anything new from MRI on this subject? Fixing for JRuby 9k only is OK for us, BTW.

Member

donv commented Dec 18, 2014

Hi @enebo ! Did you learn anything new from MRI on this subject? Fixing for JRuby 9k only is OK for us, BTW.

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Dec 18, 2014

Member

No I have not learned anything more on this. It is weird though that MRI works and we don't since they also do not add this extra info to the AST. Their lexer is completely different so they may be recording something we don't or can't.

Member

enebo commented Dec 18, 2014

No I have not learned anything more on this. It is weird though that MRI works and we don't since they also do not add this extra info to the AST. Their lexer is completely different so they may be recording something we don't or can't.

@enebo enebo modified the milestone: JRuby 9.0.0.0 Jul 14, 2015

@deivid-rodriguez

This comment has been minimized.

Show comment
Hide comment
@deivid-rodriguez

deivid-rodriguez Jul 18, 2015

Hi, has there been any progress here? I see the milestone has been modified to 9.0.0.0.

Big thanks!

Hi, has there been any progress here? I see the milestone has been modified to 9.0.0.0.

Big thanks!

@deivid-rodriguez

This comment has been minimized.

Show comment
Hide comment
@deivid-rodriguez

deivid-rodriguez Jul 18, 2015

Ooops, the milestone was actually removed... so I guess I shouldn't expect this in 9.0.0.0, right?

Ooops, the milestone was actually removed... so I guess I shouldn't expect this in 9.0.0.0, right?

@donv

This comment has been minimized.

Show comment
Hide comment
@donv

donv Mar 18, 2016

Member

Maybe this can be looked at for JRuby 9.1.0.0 ?

Member

donv commented Mar 18, 2016

Maybe this can be looked at for JRuby 9.1.0.0 ?

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Mar 21, 2016

Member

Ok I looked a few more minutes on this. So one new piece of data is that TracePoint events DO NOT have anything to do with how simple_cov keeps track of this as:

file_name = File.expand_path('dummy.rb')
File.write(file_name, "# Comment\n\n")
puts "Line count: #{File.readlines(file_name).size}"

trace = TracePoint.new() do |tp|
  puts "LINE: #{tp.path}:#{tp.lineno} - #{tp.event}" if tp.path =~ /#{file_name}/
end

trace.enable
require file_name.chomp('.rb')

will return no events at all for the empty file. If you add a call or some actual code into the file then you see events. So it is probably doing something else like perhaps using SCRIPT_LINES or possibly something with loaded features to reparse the file somehow? I guess looking at simple_cov support would be next.

Member

enebo commented Mar 21, 2016

Ok I looked a few more minutes on this. So one new piece of data is that TracePoint events DO NOT have anything to do with how simple_cov keeps track of this as:

file_name = File.expand_path('dummy.rb')
File.write(file_name, "# Comment\n\n")
puts "Line count: #{File.readlines(file_name).size}"

trace = TracePoint.new() do |tp|
  puts "LINE: #{tp.path}:#{tp.lineno} - #{tp.event}" if tp.path =~ /#{file_name}/
end

trace.enable
require file_name.chomp('.rb')

will return no events at all for the empty file. If you add a call or some actual code into the file then you see events. So it is probably doing something else like perhaps using SCRIPT_LINES or possibly something with loaded features to reparse the file somehow? I guess looking at simple_cov support would be next.

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Mar 21, 2016

Member

heh...coverage:

file_name = File.expand_path('dummy.rb')
File.write(file_name, "# Comment\n\nhash\n\n")
puts "Line count: #{File.readlines(file_name).size}"

require 'coverage'

Coverage.start
require file_name.chomp('.rb')
p Coverage.result

So it appears if we have code we create the result array. This result array is nearly right, but there seems to be two related issues:

if I have code with no code and only empty lines (e.g. comments or whitespace) like '# c\n\n\n' then JRuby will return nothing at all. Like the file does not exist.

If we have code but have trailing empty lines then the hash entry is made but the trailing lines are never added to the list.

Member

enebo commented Mar 21, 2016

heh...coverage:

file_name = File.expand_path('dummy.rb')
File.write(file_name, "# Comment\n\nhash\n\n")
puts "Line count: #{File.readlines(file_name).size}"

require 'coverage'

Coverage.start
require file_name.chomp('.rb')
p Coverage.result

So it appears if we have code we create the result array. This result array is nearly right, but there seems to be two related issues:

if I have code with no code and only empty lines (e.g. comments or whitespace) like '# c\n\n\n' then JRuby will return nothing at all. Like the file does not exist.

If we have code but have trailing empty lines then the hash entry is made but the trailing lines are never added to the list.

@enebo enebo closed this in b2a0619 Mar 21, 2016

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Mar 21, 2016

Member

@donv is going to look at adding some specs for this in ruby/spec. The commit explains the base issue and how it was addressed.

Member

enebo commented Mar 21, 2016

@donv is going to look at adding some specs for this in ruby/spec. The commit explains the base issue and how it was addressed.

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

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

Fixes #1981. Wrong number of reported lines in Coverage API
This ended up being much simpler than I think any of us thought.
Base problem was any lines after last *newline* marked node would
not update the primitive 'coverage' array which coverage sets up.
So we just call one method at end up parse to update that array
to include final lines of the file.

@enebo enebo added the parser label Mar 21, 2016

@ttwo32 ttwo32 referenced this issue in holidays/holidays Sep 5, 2016

Merged

Add jruby's debug option for test. #219

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