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

Add SimpleCov support to the test suite #2269

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

etagwerker
Copy link

Hi there,

This PR will add a new dependency to the test group and require SimpleCov if COVERAGE=true when running the test suite.

This will generate a code coverage report that will look like this:

Screen Shot 2022-09-26 at 4 42 40 PM

Screen Shot 2022-09-26 at 4 42 47 PM

Please let me know what you think.

Thanks!

require "simplecov"
SimpleCov.start "rails"

puts "Required SimpleCov"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can remove this second printout? 🙂

This will add a new dependency to the test group and require SimpleCov if COVERAGE=true when running the test suite.
@etagwerker
Copy link
Author

@pablobm Thanks for your review! I just addressed your comment. 😄

@nickcharlton
Copy link
Member

I'm not convinced that this is all that helpful for us. Do others feel a strong need for code coverage reports?

@pablobm
Copy link
Collaborator

pablobm commented Oct 20, 2022

@nickcharlton - I personally like the idea, but feel that it risks becoming a "hidden" feature unless made more apparent somehow.

@etagwerker
Copy link
Author

@nickcharlton @pablobm Having support for simplecov means that you can quickly see which files are lacking coverage:

According to the report I ran for this branch it looks like it is almost perfect:

Screen Shot 2022-10-24 at 10 33 13 PM

If you wanted to, it would make it easier to enable something like codecov to quickly assess whether a PR is increasing/decreasing code coverage by the library's test suite. So it might save you time assessing whether a change/patch has enough test cases or not.

@pablobm
Copy link
Collaborator

pablobm commented Nov 15, 2022

I think that adding some sort of warning in the GitHub checks (can you add warnings that don't block the merge?) as well as a coverage badge on the README can help raising awareness that we do want contributions to be well tested. Or perhaps it's a distraction? Thoughts, @nickcharlton?

@nickcharlton
Copy link
Member

can help raising awareness that we do want contributions to be well tested

Oh yeah, that's very true and something I'd not considered. Even though we do already have good coverage, like @etagwerker helpfully points out, we do often have to ask.

I think a GitHub Action that operates like a linter would be the nicest approach, but I couldn't find any existing ones. I did see this, but I think I'd prefer if it didn't comment if at all possible: https://blog.dennisokeeffe.com/blog/2022-03-12-simplecov-with-ruby-and-github-actions

We already don't block merges if Actions fail (which is my general preference — both here and elsewhere, I like to trust people won't hit the merge button if there are failures right about it but still ensure it's possible for when you need to)

@etagwerker
Copy link
Author

@nickcharlton A quick way to see SimpleCov's output in the console would be to combine these two things:

  1. Add simplecov-console to this PR -> https://github.com/chetan/simplecov-console
  2. Add an environment variable in here: https://github.com/thoughtbot/administrate/blob/main/.circleci/config.yml (ie. COVERAGE=true)

Then every PR would output the code coverage percentage at the end of its execution.

If you wanted to go the CodeCov route, you'd need to set up that service and then configure Circle CI to have the API key as a secret.

The comments would look like this: fastruby/skunk#109 (comment)

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.

3 participants