Skip to content

Consider prism in coverage results #2079

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Consider prism in coverage results #2079

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8f17849
Run internal investigation once
Earlopain Apr 14, 2025
c7a5392
Move prism to the main workflow matrix
Earlopain Apr 14, 2025
4f91894
Merge coverage results
Earlopain Apr 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions .github/workflows/linting.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,17 @@ concurrency:
group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }}
cancel-in-progress: true
jobs:
internal_investigation:
runs-on: ubuntu-latest
name: Internal Investigation
steps:
- uses: actions/checkout@v4
- uses: ruby/setup-ruby@v1
with:
ruby-version: ruby # Latest stable CRuby version
bundler-cache: true
- run: bundle exec rake internal_investigation

yamllint:
name: Yamllint
runs-on: ubuntu-latest
Expand All @@ -18,6 +29,7 @@ jobs:
yamllint_comment: true
env:
GITHUB_ACCESS_TOKEN: ${{ secrets.GITHUB_TOKEN }}

mdformat:
name: Mdformat
runs-on: ubuntu-latest
Expand Down
54 changes: 25 additions & 29 deletions .github/workflows/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,28 +35,45 @@ jobs:
- "3.4"
- ruby-head
- jruby-9.4
task:
- internal_investigation
- spec
name: "Ruby ${{ matrix.ruby }}: ${{ matrix.task }}"
parser_engine:
- parser_whitequark
include:
- ruby: "2.7" # Minimum version required for Prism to run.
parser_engine: parser_prism
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/rubocop/rubocop/blob/d8448a3790a78f16536e267ed7ea5ffc2e03561b/docs/modules/ROOT/pages/compatibility.adoc#L44

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

name: "Ruby ${{ matrix.ruby }} - Spec (${{ matrix.parser_engine }})"
steps:
- uses: actions/checkout@v4
- uses: ruby/setup-ruby@v1
with:
ruby-version: "${{ matrix.ruby }}"
bundler-cache: true
- run: NO_COVERAGE=true bundle exec rake ${{ matrix.task }}
- run: bundle exec rake spec
env:
PARSER_ENGINE: ${{ matrix.parser_engine }}
- name: Upload Coverage Artifact
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

uses: actions/upload-artifact@v4
with:
name: coverage-ubuntu-${{ matrix.ruby }}-${{ matrix.parser_engine }}
path: coverage/.resultset.json
include-hidden-files: true

coverage:
name: Check Coverage
needs: main
runs-on: ubuntu-latest
name: "Test coverage"

steps:
- uses: actions/checkout@v4
- uses: actions/download-artifact@v4
name: Download Coverage Artifacts
with:
pattern: coverage-*
- uses: ruby/setup-ruby@v1
with:
ruby-version: "3.4"
ruby-version: ruby # Latest stable CRuby version
bundler-cache: true
- run: bundle exec rake spec

- run: bundle exec rake coverage:ci

edge-rubocop:
runs-on: ubuntu-latest
Expand Down Expand Up @@ -122,24 +139,3 @@ jobs:
ruby-version: "3.4"
bundler-cache: true
- run: NO_COVERAGE=true bundle exec rake spec

prism:
runs-on: ubuntu-latest
name: Prism
steps:
- uses: actions/checkout@v4
- name: Use prism parser
run: |
cat << EOF > Gemfile.local
gem 'prism'
EOF
- name: set up Ruby
uses: ruby/setup-ruby@v1
with:
# Specify the minimum Ruby version 2.7 required for Prism to run.
ruby-version: "2.7"
bundler-cache: true
- env:
NO_COVERAGE: true
PARSER_ENGINE: parser_prism
run: bundle exec rake spec
2 changes: 1 addition & 1 deletion spec/spec_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
require 'rubocop'
require 'rubocop/rspec/support'

require 'simplecov' unless ENV['NO_COVERAGE']
require 'simplecov' unless ENV['NO_COVERAGE'] || RUBY_ENGINE == 'jruby'

module SpecHelper
ROOT = Pathname.new(__dir__).parent.freeze
Expand Down
10 changes: 10 additions & 0 deletions tasks/coverage.rake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# frozen_string_literal: true

namespace :coverage do
desc 'Report Coverage from merged CI runs'
task :ci do
require 'simplecov'

SimpleCov.collate Dir['coverage-*/.resultset.json']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this combine results from different Ruby versions, and Parser results with Prism results?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end
end
Loading