Skip to content
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

Support coverage for eval #1037

Merged
merged 2 commits into from
Dec 23, 2022
Merged

Conversation

mame
Copy link
Contributor

@mame mame commented Dec 3, 2022

Since Ruby 3.2, coverage.so can measure coverage of code that evaluated by Kernel#eval. Typically, this would be useful for ERB.

https://bugs.ruby-lang.org/issues/19008

This change adds a new API for SimpleCov to enable the feature.

SimpleCov.start do
  enable_coverage_for_eval
end

Here is a sample output:

image

I hope this feature will be released until the release date of Ruby 3.2 (25th in this month).

Since Ruby 3.2, coverage.so can measure coverage of code that evaluated
by `Kernel#eval`. Typically, this would be useful for ERB.

https://bugs.ruby-lang.org/issues/19008

This change adds a new API for SimpleCov to enable the feature.

```ruby
SimpleCov.start do
  enable_coverage_for_eval
end
```
@mame mame mentioned this pull request Dec 3, 2022
@mame
Copy link
Contributor Author

mame commented Dec 8, 2022

How about this PR? Ruby 3.2.0 will be released in a little over two weeks. Sorry to be so selfish, but I hope simplecov will be released in time.

@mame
Copy link
Contributor Author

mame commented Dec 14, 2022

@colszowka Sorry for the name calling. What do you think of this?

SimpleCov is the de facto coverage measurement tool for Ruby. Unless SimpleCov supports coverage for eval, there is no point in providing the feature. I would appreciate your feedback.

@colszowka
Copy link
Collaborator

@mame Hey, thanks for the PR, this looks very nice! I saw it last week already but didn't yet have the time to take a closer look unfortunately, but I will take a look in the next days and will cut a new release in time before ruby day 👍

@mame
Copy link
Contributor Author

mame commented Dec 16, 2022

@colszowka Thank you! If there's anything I can do to help, feel free to let me know.

@colszowka colszowka merged commit 6bc65b0 into simplecov-ruby:main Dec 23, 2022
@colszowka
Copy link
Collaborator

Hey @mame, v0.22.0 featuring this and #1035 are now out on rubygems, thanks again for your contributions!

I have two questions regarding this new feature:

  • Since Rails uses erubis, this won't work on Rails ".erb" templates out of the box, or will it?
  • Is there some documentation on how Coverage requires this filename to be set, and if so, could you please point me there? I suppose there is some integration between Coverage and ERB that makes this work built into 3.2, but how could I use this for example with just a plain eval statement in my code, that I've read from some arbitrary ruby file?

@mame
Copy link
Contributor Author

mame commented Dec 24, 2022

Hey @mame, v0.22.0 featuring this and #1035 are now out on rubygems, thanks again for your contributions!

Thank you very much!

I have two questions regarding this new feature:

  • Since Rails uses erubis, this won't work on Rails ".erb" templates out of the box, or will it?

I am not familiar with the implementation of Rails. It works on Rails out of the box by the following procedure:

$ ruby -v
ruby 3.2.0dev (2022-12-24T02:29:11Z master d5635dfe36) [x86_64-linux]

$ gem install rails

$ rails new myapp

$ cd myapp

# Add `gem "simplecov"` in the "test" group of Gemfile
# (and temporarily you need to tweak the ruby version to "3.2.0.dev")

$ vi Gemfile

$ bundle

$ bundle exec rails generate controller Home index

# Add the following two lines to the top of test_helper
#   require "simplecov"
#   SimpleCov.start { enable_coverage_for_eval }

$ vi test/test_helper.rb

$ bundle exec rails test

And you will be able to see the coverage of "app/views/layouts/application.html.erb" in coverge/index.html.

  • Is there some documentation on how Coverage requires this filename to be set, and if so, could you please point me there?

Hm, indeed it is not documented yet. The simplest answer is that Coverage handles the third argument of Kernel#eval as a filename of the code.
@ioquatix I am happy if you could work on it. (I think it is okay after the release of Ruby 3.2.0.)

I suppose there is some integration between Coverage and ERB that makes this work built into 3.2, but how could I use this for example with just a plain eval statement in my code, that I've read from some arbitrary ruby file?

ERB provides ERB#filename= to specify the filename to be passed to Kernel#eval. The point is that ERB keeps the line numbers when compiling erb code to generated ruby code. So Coverage can measure the coverage of the generated ruby code as if it is the coverage of the original erb code.

@mame mame deleted the coverage-for-eval branch December 24, 2022 05:32
@colszowka
Copy link
Collaborator

@mame Thanks for the quick reply!

Regarding rails, my understanding was that rails was using erubis instead of irb, and I saw that it's still a dependency of Actionpack, so I assumed that this would mean that it wouldn't work out of the box there, but if it works already that's very cool, thanks for the clarification 🎉! I wanted to try it out myself yesterday but had some issue installing ruby head locally and ran into some CI issues that were blocking me from publishing the new version, so I focused on that.

ERB provides ERB#filename= to specify the filename to be passed to Kernel#eval. The point is that ERB keeps the line numbers when compiling erb code to generated ruby code. So Coverage can measure the coverage of the generated ruby code as if it is the coverage of the original erb code.

I think having some basic example code on how to integrate with this would be really nice, I think it would suffice to show how to pass the neccessary arguments to eval for an arbitrary eval code block, as a reference for other libraries i.e. for templates on how to integrate this 📒 We could then also point to this from the readme docs on here for anyone interested in integrating with that

timpeat added a commit to ministryofjustice/laa-review-criminal-legal-aid that referenced this pull request Feb 15, 2023
This is now possible with ruby 3.2.1 as per:
simplecov-ruby/simplecov#1037

Add specs for missing coverage.
timpeat added a commit to ministryofjustice/laa-review-criminal-legal-aid that referenced this pull request Feb 15, 2023
This is now possible with ruby 3.2.1 as per:
simplecov-ruby/simplecov#1037

Add specs for missing coverage.
timpeat added a commit to ministryofjustice/laa-review-criminal-legal-aid that referenced this pull request Feb 15, 2023
This is now possible with ruby 3.2.1 as per:
simplecov-ruby/simplecov#1037

Add specs for missing coverage.
timpeat added a commit to ministryofjustice/laa-review-criminal-legal-aid that referenced this pull request Feb 15, 2023
This is now possible with ruby 3.2.1 as per:
simplecov-ruby/simplecov#1037

Add specs for missing coverage.
timpeat added a commit to ministryofjustice/laa-review-criminal-legal-aid that referenced this pull request Feb 15, 2023
This is now possible with ruby 3.2.1 as per:
simplecov-ruby/simplecov#1037

Add specs for missing coverage.
timpeat added a commit to ministryofjustice/laa-review-criminal-legal-aid that referenced this pull request Feb 15, 2023
This is now possible with ruby 3.2.1 as per:
simplecov-ruby/simplecov#1037

Add specs for missing coverage.
arthurashman pushed a commit to ministryofjustice/laa-review-criminal-legal-aid that referenced this pull request Feb 16, 2023
This is now possible with ruby 3.2.1 as per:
simplecov-ruby/simplecov#1037

Add specs for missing coverage.
toshimaru added a commit to toshimaru/RailsTwitterClone that referenced this pull request Feb 19, 2023
@joshuapinter
Copy link
Contributor

After enabling this via enable_coverage_for_eval, I'm seeing a bunch of these messages after running tests:

Warning: coverage data provided by Coverage [271] exceeds number of lines in ...

I've disabled it for now but let me know if there's anything I can do to help diagnose.

@ioquatix
Copy link

It probably means something is wrong with the template (erb has some known issues).

@joshuapinter
Copy link
Contributor

@ioquatix Thanks for the quick reply - didn't expect anything back. :)

When you say "something is wrong", is it something that we've done with the template or do you mean just that simplecov can't handle erb templates very well?

@ioquatix
Copy link

Last time I checked, there were some issues with erb leaving things on the correct line, let me see if I can find my repro.

@ioquatix
Copy link

ioquatix commented Jun 1, 2023

Since you have not provided your templates, I cannot know for sure what is going on. However, my guess is an ERB template with 5 lines of input code, generates 6 lines of "compiled template".

I've made some code to play around with this:

image

I added a quick hack to print out the number of lines of code and the number of coverage counts captured. You can see a difference 8 vs 9. See the files here: https://github.com/ioquatix/covered/tree/main/examples/coverage/erb for the code.

In terms of a probable fix for simplecov, I would recommend ignoring any coverage beyond the end of the file (or before line 1 too). Some template systems take advantage of "A file consists of lines 1..n" and use instance_eval(..., line=-1) to put a few extra lines of code at the start or end.

@joshuapinter
Copy link
Contributor

joshuapinter commented Jun 1, 2023

Hey @ioquatix, thanks for the detailed response here!

I re-ran the tests and picked out a simple one to look at as well:

Here's the .erb.html file:

<%# TODO: ... %>

<% person ||= current_user # Assume current user if nobody passed in. %>
<% style  ||= ""           # Assume no specific styles. %>

<% if person.avatar.nil? %>
  <%= image_tag "avatar_icon.png", class: "avatar circle", style: style %>

<% else %>
  <%= image_tag person.avatar.url( :medium ), class: "avatar circle", style: style %>

<% end -%>

And here is the warning message:

Warning: coverage data provided by Coverage [14] exceeds number of lines in .../_avatar.html.erb [12]

@ioquatix
Copy link

ioquatix commented Jun 2, 2023

According to your template:

     1| <%# TODO: ... %>
     2| 
     3| <% person ||= current_user # Assume current user if nobody passed in. %>
     4| <% style  ||= ""           # Assume no specific styles. %>
     5| 
     6| <% if person.avatar.nil? %>
     7| <%= image_tag "avatar_icon.png", class: "avatar circle", style: style %>
     8| 
     9| <% else %>
    10| <%= image_tag person.avatar.url( :medium ), class: "avatar circle", style: style %>
    11| 
    12| <% end -%>

ERB compiles to:

     0| #coding:UTF-8
     1| _erbout = +''; _erbout.<< "\n\n".freeze
     2| 
     3| ;  person ||= current_user # Assume current user if nobody passed in. ; _erbout.<< "\n".freeze
     4| ;  style  ||= ""           # Assume no specific styles. ; _erbout.<< "\n\n".freeze
     5| 
     6| ;  if person.avatar.nil? ; _erbout.<< "\n".freeze
     7| ; _erbout.<<(( image_tag "avatar_icon.png", class: "avatar circle", style: style ).to_s); _erbout.<< "\n\n".freeze
     8| 
     9| ;  else ; _erbout.<< "\n".freeze
    10| ; _erbout.<<(( image_tag person.avatar.url( :medium ), class: "avatar circle", style: style ).to_s); _erbout.<< "\n\n".freeze
    11| 
    12| ;  end -; _erbout.<< "\n".freeze
    13| ; _erbout

Note that I've set the ERB template to base-0 line numbers. That's because of this: https://github.com/ruby/erb/blob/3d3df5ce5c9cb9564bdee8b02b0bb4fd81981e82/lib/erb.rb#L353 which is given to https://github.com/ruby/erb/blob/3d3df5ce5c9cb9564bdee8b02b0bb4fd81981e82/lib/erb.rb#L429

There are two fixes:

  • Simplecov should ignore line 13 in the above case (i.e. any lines beyond the end of the original code).
  • ERB should consider avoiding generating extra lines at the end of the template. In other words, line 13 should be on the end of line 12 (or use tap etc).

@joshuapinter
Copy link
Contributor

@ioquatix Great analysis! This is above my head to fix but good reference for anybody that wants to take a stab at this. For now I think I'll either suppress the warning or disable the enable_coverage_for_eval. Thanks again for looking into this.

@ioquatix
Copy link

ioquatix commented Jun 2, 2023

I think you can also just ignore the warnings, no harm is caused AFAIK.

@joshuapinter
Copy link
Contributor

It's just nasty, though. :) In our case, over 100+ lines of these warnings. Makes our test output noisy.

@dla-c-box
Copy link

Does it mean that the following part of the readme is somewhat obsolete for Ruby 3.2+: gathering coverage for common templating solutions like erb, slim and haml is not supported

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants