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

Configure coverage percentage #20

Open
misteral opened this issue Feb 4, 2022 · 4 comments · May be fixed by #21
Open

Configure coverage percentage #20

misteral opened this issue Feb 4, 2022 · 4 comments · May be fixed by #21

Comments

@misteral
Copy link

misteral commented Feb 4, 2022

Hi, how I can configure percentage coverage for method and not raise warning if it above.

Current pronto message:

app/serializers/applogin_history_serializer.rb:21 W: instance method subapp_name missing tests for lines 21, 22 (coverage: 0.6)

how to hide this if I configure coverage to (0.5) - I think this is 50%

@misteral misteral linked a pull request Feb 9, 2022 that will close this issue
@grodowski
Copy link
Owner

Hey @misteral and thanks for opening #21. TBH I'm not convinced setting a % threshold is the way to ignore certain uncovered lines. Why did you choose to go for that option, versus e.g. local comments (maybe nocov will just work?) or a config file (similar to brakeman's ignore list?).

Undercover is scoping comments to methods where untested lines were changed (lines have to appear in the diff directly), so both of the latter could let you ignore lines on a case-by-case basis.

Another side-note that's been on my radar and also discussed in #15: warnings could be generated at method level as well, e.g. the lines you've changed are 100% covered, but the entire enclosing method is missing tests. Could be implemented in the form of an alternative matching/analysis mode.

Let me know what you think!

@misteral
Copy link
Author

Hi, @grotowski I'm going to implement pronto as review bot, we have project with 13% code coverage ... Not bad )) I wanna change it and we are decided doing coverage for 50+% lines of method, this is allow us move a bit faster in first refactoring circle.

@grodowski
Copy link
Owner

Sure, I understand your perspective, you might not want to test everything or deliberately skip some coverage. I think you could stick to the "50+% line coverage per method" requirement by still just using the nocov tags appropriately - keep in mind that undercover does not trigger warnings on every method, just those where at least one changed line is missing tests.

If that happens in a commit/PR, wrap the lines in a nocov and... profit? You could also get back to all the nocovs in the future to add missing tests.

Here's an example I played with recently to check if nocov can successfully silence undercover warnings - it can :)

# :nocov:
if false == :true
  puts "I'm untested"
  end
# :nocov:

Btw this is my reasoning, I am a bit sceptical about merging in min-coverage as a concept. Please let me know your thoughts

@misteral
Copy link
Author

Hi, yes I am clear with your approach, agree with you in case human review, but we use auto PR review for now and we should make decisions fast, we are currently use my fork, please decide about this PR on your side, thanks @grodowski .

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 a pull request may close this issue.

2 participants