New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Coverage levels discrepancies between JRuby 1.7.x and JRuby 9.1.x #4819

Closed
montycheese opened this Issue Oct 16, 2017 · 9 comments

Comments

Projects
None yet
4 participants
@montycheese

montycheese commented Oct 16, 2017

After upgrading my Rails app's JRuby version from JRuby 1.7.27 to JRuby 9.1.13 I noticed a drop in coverage levels in my code base from 100% to roughly 94%.

I'm using the SimpleCov Gem to output the rspec coverage but it uses Ruby's underlying Coverage module to get the actual coverage numbers.
(rspec 3.6 and simplecov 0.15)

Environment

JRuby 9.1.13

  • rspec 3.6 and simplecov 0.15

  • Rails

Expected Behavior

Behavior prior to upgrade
pre-upgrade

Behavior in ruby 2.4.1 using (rbenv)
post-upgrade-nonjruby

Actual Behavior

Behavior post upgrade to JRuby 9.1.13
post-upgrade

Here's a screen shot of a happy test to prove that testing does indeed work in my JRuby 9.1.13 setup (removed the instance variable)
post-upgrade-happy

As seen in the screen shots of the simple test above, coverage is incorrectly skipped in jruby9000 in certain cases (instance variable in a boolean condition in the above case)

I can post more examples if necessary for debugging.

Am I just missing some special flag when calling rspec, or is there something bigger going on here?
I've run with --dev, --debug, and -Xcli.debug=true

@enebo enebo added the ir label Oct 17, 2017

@enebo enebo added this to the JRuby 9.1.14.0 milestone Oct 17, 2017

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Oct 17, 2017

Member

Nope. We just have a bug here. JRuby 9k uses completely different internals than JRuby 1.7.x. JRuby 9k now converts a syntax tree into a set of virtual machine instrs and emits a line_num instr. In this case I am guessing the instrance variable + if ends up putting a line number instr in a place where we think we can cull it (or never emit it in the first place).

Member

enebo commented Oct 17, 2017

Nope. We just have a bug here. JRuby 9k uses completely different internals than JRuby 1.7.x. JRuby 9k now converts a syntax tree into a set of virtual machine instrs and emits a line_num instr. In this case I am guessing the instrance variable + if ends up putting a line number instr in a place where we think we can cull it (or never emit it in the first place).

@mullermp

This comment has been minimized.

Show comment
Hide comment
@mullermp

mullermp Oct 18, 2017

@enebo To add to this, there are other examples of this failed coverage, not just for 'if else' with instance variables. I saw a similar issue with .to_json() not getting covered. It's a more general problem I think, not limited to this example.

mullermp commented Oct 18, 2017

@enebo To add to this, there are other examples of this failed coverage, not just for 'if else' with instance variables. I saw a similar issue with .to_json() not getting covered. It's a more general problem I think, not limited to this example.

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Oct 19, 2017

Member

@mullermp yeah it would not surprise me very much. In fact it probably has nothing to do with the if/else specifically and more to do with us emitting line_num in combination of two elements (like if and an instance variable). I have fixed multiple coverage issues already so either this is a regression or oddly a case with no coverage in our tests.

Member

enebo commented Oct 19, 2017

@mullermp yeah it would not surprise me very much. In fact it probably has nothing to do with the if/else specifically and more to do with us emitting line_num in combination of two elements (like if and an instance variable). I have fixed multiple coverage issues already so either this is a regression or oddly a case with no coverage in our tests.

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Oct 19, 2017

Member

Ok looked into this and wowsers it was not in IR at all. It was back in the AST. Basically anything which represents an ident during lexing ivar, cvar, gvar, keywords, constants would ask lexer for current position but lexer will advance past the ident in cases where the ident is last thing on a line and advance to next line (not 100% of the time but most of the time).

Fortunately, the lexer has a special position value for idents called tokline. I switched all obvious nodes made directly from idents to use tokline and it seems to have fixed this issue and probably many others.

I also ran simplecov specs and while there were 4 errors....none were related to positions changing. Thinking about whether we have any nice tests for this somewhere else too. Obviously this is not being captured in any test runs we normally do.

Member

enebo commented Oct 19, 2017

Ok looked into this and wowsers it was not in IR at all. It was back in the AST. Basically anything which represents an ident during lexing ivar, cvar, gvar, keywords, constants would ask lexer for current position but lexer will advance past the ident in cases where the ident is last thing on a line and advance to next line (not 100% of the time but most of the time).

Fortunately, the lexer has a special position value for idents called tokline. I switched all obvious nodes made directly from idents to use tokline and it seems to have fixed this issue and probably many others.

I also ran simplecov specs and while there were 4 errors....none were related to positions changing. Thinking about whether we have any nice tests for this somewhere else too. Obviously this is not being captured in any test runs we normally do.

enebo added a commit that referenced this issue Oct 19, 2017

Fixes #4819. Coverage levels discrepancies between JRuby 1.7.x and JR…
…uby 9.1.x

Basically anything which represents an ident during lexing ivar, cvar, gvar, keywords, constants would ask lexer for current position but lexer will advance past the ident in cases where the ident is last thing on a line and advance to next line (not 100% of the time but most of the time).

Fortunately, the lexer has a special position value for idents called tokline. I switched all obvious nodes made directly from idents to use tokline and it seems to have fixed this issue and probably many others.

@enebo enebo closed this in f0b0360 Oct 19, 2017

@mullermp

This comment has been minimized.

Show comment
Hide comment
@mullermp

mullermp Oct 19, 2017

Awesome. I'll be trying this after release and I'll re-open if there's still any issues with coverage.

mullermp commented Oct 19, 2017

Awesome. I'll be trying this after release and I'll re-open if there's still any issues with coverage.

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Oct 19, 2017

Member

@mullermp yeah fantastic. We want perfect compat for coverage and will prioritize if we can.

Member

enebo commented Oct 19, 2017

@mullermp yeah fantastic. We want perfect compat for coverage and will prioritize if we can.

@zendes

This comment has been minimized.

Show comment
Hide comment
@zendes

zendes Nov 9, 2017

looks like this fix doesn't cover all cases. here is an example of a constant being tested that gets 100% in MRI 2.4.2 and 0% in JRuby9000:

code:
testcode

MRI 2.4.2:
mri2 4 2

JRuby9000:
jruby9000

zendes commented Nov 9, 2017

looks like this fix doesn't cover all cases. here is an example of a constant being tested that gets 100% in MRI 2.4.2 and 0% in JRuby9000:

code:
testcode

MRI 2.4.2:
mri2 4 2

JRuby9000:
jruby9000

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Nov 9, 2017

Member

@zendes can you open this up as a new issue please?

Member

enebo commented Nov 9, 2017

@zendes can you open this up as a new issue please?

@montycheese

This comment has been minimized.

Show comment
Hide comment
@montycheese

montycheese Dec 14, 2017

I think this is part of the same issue. The original example I posted were just a simple one. I have issues such as coverage missing in multi-line hashes. And multi-line if statements conditions that are missed but can be resolved by editing the condition.

e.g.

foo << bar unless bool1 || #HIT
bool2 || #MISS
string1 != string2 #MISS

foo << bar unless bool1 || #HIT
!!(bool2 == true) || #HIT
!(string1 == string2) #HIT

montycheese commented Dec 14, 2017

I think this is part of the same issue. The original example I posted were just a simple one. I have issues such as coverage missing in multi-line hashes. And multi-line if statements conditions that are missed but can be resolved by editing the condition.

e.g.

foo << bar unless bool1 || #HIT
bool2 || #MISS
string1 != string2 #MISS

foo << bar unless bool1 || #HIT
!!(bool2 == true) || #HIT
!(string1 == string2) #HIT

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