Consider prism in coverage results #2079
Conversation
Makes it so that the main matrix only runs a single task
rubocop-ast depends on prism, so no need to add it to the gemfile anymore
Earlopain
force-pushed
the
prism-coverage-merge
branch
2 times, most recently
from
60a5c3e to
11b6f48
Compare
.github/workflows/linting.yml
Outdated
| with: | ||
| ruby-version: ruby # Latest stable CRuby version | ||
| bundler-cache: true | ||
| - run: NO_COVERAGE=true bundle exec rake internal_investigation |
There was a problem hiding this comment.
Internal investigation shouldn't be bothered by NO_COVERAGE, or why is it?
There was a problem hiding this comment.
You're right, I had this from the main job where I extracted this from. Will remove
| - run: bundle exec rake spec | ||
| env: | ||
| PARSER_ENGINE: ${{ matrix.parser_engine }} | ||
| - name: Upload Coverage Artifact |
There was a problem hiding this comment.
I think we're quite fine if it just fails.
Also, it's fine if it runs for just one Ruby version, which is done below in the "coverage" job (line 50)
@rubocop/rubocop-rspec WDYT?
There was a problem hiding this comment.
I think we're quite fine if it just fails.
Yeah, I can do that. if-no-files-found was a bit nice during debuging but not really necessary anymore (plus jruby kinda needs that, or something similar)
| - parser_whitequark | ||
| include: | ||
| - ruby: "2.7" # Minimum version required for Prism to run. | ||
| parser_engine: parser_prism |
There was a problem hiding this comment.
Starting from RuboCop 1.75, the prism translation layer is used by default when analyzing Ruby 3.4+.
What is the point for us to run this job on 2.7, and tell the Prism to analyze the code as it was Ruby 3.4? Don't we have those 3.4-specific (eg it-block) specs disabled when running on Ruby 2.7?
Is it just my OCD, or we need two Prism jobs, one on 2.7, to ensure it runs on older rubies, and one on 3.4, to ensure cops do what we expect them to, including new features, with Prism?
There was a problem hiding this comment.
What is the point for us to run this job on 2.7, and tell the Prism to analyze the code as it was Ruby 3.4?
It's just the ruby version that it is being run on. So even with a runtime of Ruby 2.7 it will analyze it as syntax from those ruby versions like parser would do it (in practise it will "upgrade" 2.7 and other versions it can't anaylze to 3.3, which is the lowest ruby version that prism understands)
Is it just my OCD, or we need two Prism jobs, one on 2.7, to ensure it runs on older rubies, and one on 3.4, to ensure cops do what we expect them to, including new features, with Prism?
It's the lowest to not accidentally use newer ruby features like you mentioned. It will still analyze such newer ruby versions even with a lower runtime ruby.
| task :ci do | ||
| require 'simplecov' | ||
|
|
||
| SimpleCov.collate Dir['coverage-*/.resultset.json'] |
There was a problem hiding this comment.
Will this combine results from different Ruby versions, and Parser results with Prism results?
There was a problem hiding this comment.
Yes. Different ruby versions aren't really needed since it will be at 100% with just two jobs right now (one prism, one parser) but limiting it in the workflow adds complexity which I don't think is really needed.
Just doing it will all runs seems ok to me for simplicity.
parser and prism will differ in what they do, so merge them together. jruby doesn't seem to support coverage (maybe jruby bug?): > KeyError: key not found: :branch
Earlopain
force-pushed
the
prism-coverage-merge
branch
from
11b6f48 to
4f91894
Compare
In #2077, I want to add prism-only logic that won't be executed by the
parsergem. Coverage complains, because it thinks some code is unreachable.This PR fixes this by making a few changes to CI, with the goal to merge the coverage results so it can get back to 100%.
I made the following changes:
internal_investigtionis not really for catchingrubocop-rspecerrors, there's the test suite for that. So running it only once is ok (I made the same change inrubocopa while ago). Doing this simplifies merging since only a single task is executed.rubocop-astpulls prism in. Also simplifies merging coverageIf this is merged, I can rebase #2077 and CI should be green there. I tested this locally with coverage results from the different parser engines and it reports 100% coverage after merging and behaves like I would expect it to.