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

Detect and warn about incorrectly used ignore-comments #325

Merged
merged 9 commits into from
May 30, 2024

Conversation

RKushnir
Copy link
Contributor

@RKushnir RKushnir commented Mar 27, 2024

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
Copy link
Owner

parroty commented Apr 1, 2024

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
Copy link
Contributor Author

RKushnir commented Apr 1, 2024

@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
Copy link
Owner

parroty commented Apr 15, 2024

@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
Copy link
Contributor Author

@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
Copy link
Contributor Author

@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
Copy link
Owner

parroty commented May 27, 2024

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

@parroty parroty merged commit a530769 into parroty:master May 30, 2024
2 checks passed
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.

Fail coverage if any file contains an unmatched coveralls-ignore-start
2 participants