parroty / excoveralls

Coverage report tool for Elixir with coveralls.io integration.
MIT License
820 stars 178 forks source link

Detect and warn about incorrectly used ignore-comments #325

Closed RKushnir closed 3 months ago

RKushnir commented 5 months ago

This PR addresses the issue of accidentally ignoring too much code coverage. The issue happens when you add coveralls-ignore-start but forget to add the corresponding -ignore-stop. With this change, excoveralls is going to discard the un-closed ignore-block (i.e. not ignore the coverage) and give you a warning, so that you can more easily spot the mistake.

Besides the main goal, this change also adds a few other useful warnings to let you know about redundant or invalid ignore-comments, which I previously outlined. This is an optional part, and we could remove it or split it into a separate PR if that would be more appropriate.

The most interesting part for review is ignore.ex and ignore_test.exs.

Here's what the warnings look like in the console output image

The PR is unfinished, it's missing some of the final touches: some tests, changelog entry, version bump, readme update. But it's feature-complete and so I would love to get early feedback, in case I need to change anything before I polish it. I'm looking forward to your thoughts and suggestions.

Resolves #197.

parroty commented 5 months ago

Thank you for the PR 🙇 . I think it makes sense to emit warning for avoiding unexpected behavior. It would be beneficial as long as existing behavior is maintained (just providing warning and doesn't throw error?)

RKushnir commented 5 months ago

@parroty Thanks for the feedback. What about the coverage? Should we ignore coverage if there is only ignore-start without ignore-stop? (existing behavior)

parroty commented 4 months ago

@RKushnir I believe it's already a large change, so I am wondering taking a reasonably small step (while maintaining existing behavior) would be an option? (rather than extending coverage?), but looking for your opinion, too.

RKushnir commented 4 months ago

@parroty I agree. I think it's already a good progress if we just show the warnings. I'll try to update the PR soon.

Later we can add some config option e.g. --warnings-as-errors to fail the build if there are any warnings.

RKushnir commented 4 months ago

@parroty I updated the PR: reverted the behavior change as we discussed, added more tests and a changelog entry. Unless I missed something, it should be ready now. Thank you for your help.

parroty commented 3 months ago

Thanks and very sorry about taking time to merge. I'll try merge and release during this week 🙇 .