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

Adds undercover gem to the stack #12720

Conversation

filipefurtad0
Copy link
Contributor

@filipefurtad0 filipefurtad0 commented Jul 31, 2024

What? Why?

(Please note: do not try to install this gem with a conda environment running ❗ )

Adds undercover gem to the stack and runs it within every CI node - by adding a Check code coverage changes task:

image

If there are no changes, we should see:

image

This, however, is not seen on the first pic. I think the rake task is not doing what it should - it should yield the same output as when ran locallt. Any ideas here? ❓

What should we test?

There are two main use cases for this gem:

  • to find untested code changes locally (with the CLI)

To do this locally, run:
bundle exec undercover --compare origin/master

  • to have review checks on GitHub

To achieve this, a a rake task was created and called within each type on specs, within build.yml. I'm not sure this is the best approach, happy for comments/suggestions/improvements!

Ideally, we'd test both scenarios, locally and remotely (CI):

Locally

  • Commiting a code change without adding tests, on a given branch
  • Run bundle exec undercover --compare origin/master
  • One should see an output from indicating changes on code coverage

Remotely/CI

  • Push the previous commits
  • On each node, we should be able to see an output, indicating code changes

Release notes

Changelog Category (reviewers may add a label for the release notes):

  • User facing changes
  • API changes (V0, V1, DFC or Webhook)
  • Technical changes only
  • Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

@filipefurtad0 filipefurtad0 added the technical changes only These pull requests do not contain user facing changes and are grouped in release notes label Jul 31, 2024
@filipefurtad0 filipefurtad0 force-pushed the adds_undercover_gem branch 6 times, most recently from f1f0cf3 to c6a823b Compare August 6, 2024 11:18
@filipefurtad0 filipefurtad0 force-pushed the adds_undercover_gem branch 2 times, most recently from cdc0847 to c4355fe Compare August 6, 2024 13:41
@filipefurtad0
Copy link
Contributor Author

Requesting early review on this one. I can't seem to progress much more here. Issues I've found:

  • we get this warning, when running undercover on CI: parser/current is loading parser/ruby31, which recognizes 3.1.6-compliant syntax, but you are running 3.1.4.
  • I've deleted (and committed) specs, and ran undercover locally. I was expecting to see some warning on code coverage reduction (this is the opposite of adding uncovered code, but I figured it should work). Another approach could be to cherry pick some commits with no tests, from other branches

@filipefurtad0 filipefurtad0 marked this pull request as draft August 7, 2024 11:04
@filipefurtad0
Copy link
Contributor Author

I'm not sure this is a good start. If this takes too much capacity or is a too curly effort at this stage, please feel free to close this PR. Thanks! 🙏

Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

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

I think in order to compare to master, it first needs a coverage report for master. So we would need to run it on master and commit those files. Or are they uploaded somewhere? I think that it's unrealistic to keep the coverage report on master up-to-date unless it's automated. And that's really difficult with a parallelised build like we have. That's were other simplecov integrations failed in the past.

Oh, I see, we need to sign up at https://undercover-ci.com/ and it's free for open source. It could work but the parallel build may make it unreliable. Not sure if it's worth it.

namespace :undercover do
desc "Runs undercover comparison against master"
task run_diff_master: :environment do
"bundle exec undercover --compare origin/master"
Copy link
Member

Choose a reason for hiding this comment

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

What is the advantage of a rake task?

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 tried to follow the syntax or way we run tasks from build.yml, we seem to have opted for running them with bin/rake, for example:


        run: |
          bin/rake knapsack_pro:rspec

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yes. I think that's Knapsack specific. Before Knapsack, we used to run bundle exec rspec.

Comment on lines 5 to 8
ENV["RAILS_ENV"] ||= 'test'

require 'simplecov' if ENV["COVERAGE"]
require 'simplecov-lcov'
require 'simplecov' if ENV["COVERAGE"] || ENV["RAILS_ENV"]
require 'simplecov-lcov' if ENV["COVERAGE"] || ENV["RAILS_ENV"]
Copy link
Member

Choose a reason for hiding this comment

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

That's an odd condition. We set the rails env above and then it's always set. The condition is always true and therefore useless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. Perhaps it would be better to remove if ENV["COVERAGE"] when requiring 'simplecov', I think we'd need it to run undercover locally.

@dacook dacook linked an issue Aug 8, 2024 that may be closed by this pull request
Copy link
Member

@dacook dacook left a comment

Choose a reason for hiding this comment

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

Sounds like there's still a bit to work out, so maybe it's worth parking this?
It could be closed, but still viewable now that I've attached it to the issue #11948

@filipefurtad0
Copy link
Contributor Author

It could be closed, but still viewable now that I've attached it to the issue #11948

Agree, I think it's best to close it. Thank you for linking it to the issue @dacook 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
technical changes only These pull requests do not contain user facing changes and are grouped in release notes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Set up code coverage metrics
3 participants