Set up code coverage metrics with simplecov #12798
Conversation
johansenja
force-pushed
the
enable-simplecov
branch
2 times, most recently
from
b2116b0
to
715adec
Compare
|
Unfortunately, it seems like this is affected by actions/download-artifact#297 - it seems to work perfectly with v3, but with v4 none of the chunks seem to be available |
johansenja
force-pushed
the
enable-simplecov
branch
from
0be81be
to
1d7f1de
Compare
johansenja
force-pushed
the
enable-simplecov
branch
from
1d7f1de
to
85385a1
Compare
There was a problem hiding this comment.
Nice one ! I noticed the index file in archive is missing the css/js do you think they could be added to the uploaded artificact ?
I am also wondering if we could also add a link to the artifact in the PR, so it's easier to find/access , maybe something like this : https://medium.com/@vlyskouski/github-actions-auto-reply-in-pull-requests-with-a-link-to-build-artifacts-ae1ef55034d8. Would you be willing to give this a go ?
There was a problem hiding this comment.
Great work, thank you.
I can see it's all working beautifully:
95% doesn't sound too bad!
Generated 2024-08-23T10:33:26+00:00
All Files ( 95.45% covered at 1024.53 hits/line )
801 files in total.
20959 relevant lines, 20006 lines covered and 953 lines missed. ( 95.45% )
lib/tasks/simplecov.rake
Outdated
| SimpleCov.collate Dir[File.join(path_to_results, "**", ".resultset.json")], "rails" do | ||
| formatter SimpleCov::Formatter::HTMLFormatter | ||
|
|
||
| coverage_dir coverage_dir |
There was a problem hiding this comment.
I'm wondering if this was only meant to be called once?
There was a problem hiding this comment.
Calling the variable twice would be a syntax error. There's a method with the same name. It would be more obvious to write it this way:
| coverage_dir coverage_dir | |
| coverage_dir(coverage_dir) |
But I think that it would be better to rename the variable as well. Most configure blocks have the config as variable and then you would call config.coverage_dir(destination_path) but SimpleCov decided to simplify and provide methods directly in the code block.
| with: | ||
| bundler-cache: true # runs 'bundle install' and caches installed gems automatically |
There was a problem hiding this comment.
I guess we only need to use the simplecov gem, and don't need all the other gems in the gemfile.
In that case, perhaps this could be optimised. But hopefully loading all the gems from cache doesn't take too long anyway.
There was a problem hiding this comment.
Nice, I'm looking forward to these results.
This is exactly what we needed but didn't have time to implement. I wonder if that rake task would be useful to others. Maybe we could put it in a gem and publish it, alongside instructions for GH Actions to use it.
I think that it would be good to resolve the name clash in the configuration. But I would merge it anyway.
Thank you very much!
spec/lib/tasks/simplecov_spec.rb
Outdated
| require 'rake' | ||
|
|
||
| RSpec.describe "simplecov.rake" do | ||
| let(:output_dir) { "tmp/test_#{Process.pid}_coverage" } |
There was a problem hiding this comment.
This is not cleaned up after the test run. What do you think about using Dir.mktmpdir?
lib/tasks/simplecov.rake
Outdated
| SimpleCov.collate Dir[File.join(path_to_results, "**", ".resultset.json")], "rails" do | ||
| formatter SimpleCov::Formatter::HTMLFormatter | ||
|
|
||
| coverage_dir coverage_dir |
There was a problem hiding this comment.
Calling the variable twice would be a syntax error. There's a method with the same name. It would be more obvious to write it this way:
| coverage_dir coverage_dir | |
| coverage_dir(coverage_dir) |
But I think that it would be better to rename the variable as well. Most configure blocks have the config as variable and then you would call config.coverage_dir(destination_path) but SimpleCov decided to simplify and provide methods directly in the code block.
mkllnk
added
technical changes only
Best viewed ignoring whitespace changes.
A variable and a method were called the same. Also made method calls more obvious with parenthesis.
|
I just went ahead and made a couple of changes to clarify the code. In this kind of community project, it's really important to make code super-simple to avoid confusion and as fool-proof as possible. That will enable more people to contribute in the future. |
|
That's such a useful report!
That would be convenient. It would take up more space though. I just ran the coverage locally for one tiny spec and then copied the artifacts. |
|
Thanks for the feedback and revisions! I opened a small PR here as well just to resolve the styling issue: #12822 |
|
In terms of commenting on the PR, I have found https://github.com/marketplace/actions/code-coverage-summary to be quite nice in the past - it comments the report as a nice table so you don't even have to click through or download the artifacts (but they are still there if you want the more detailed view) 
|
|
That looks like a good option. Happy to try out the summary comments. Are you up for adding it? |
What? Why?
The complexity here clearly comes from the parallelism in the tests with knapsack; each of the portions of tests runs in its own runner with its own disk, so the different runners don't have access to the same filesystem.
My solution here is that each of the runners still uploads its own test results, and then there is a separate job which runs afterwards, which downloads each of the results and combines them into one (similar to the recommended steps in https://docs.knapsackpro.com/ruby/simplecov/#how-to-merge-simplecov-reports-from-parallel-ci-nodes, https://github.com/simplecov-ruby/simplecov/tree/main?tab=readme-ov-file#merging-test-runs-under-different-execution-environments and simplecov-ruby/simplecov#986 (comment)). The main drawback of this approach is that there would now be loads of artifacts, which is a bit more noise when it comes to accessing the useful ones (screenshots, and the final combined report)
This could open the doorway for other features too, such as badges, undercover, adding a PR comment with a coverage report, designating simplecov groups, setting thresholds etc.
What should we test?
Release notes
Changelog Category (reviewers may add a label for the release notes):
The title of the pull request will be included in the release notes.
Dependencies
Documentation updates