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

Fail coverage if any file contains an unmatched coveralls-ignore-start #197

Open
tonyvanriet opened this issue Sep 30, 2019 · 5 comments · May be fixed by #325
Open

Fail coverage if any file contains an unmatched coveralls-ignore-start #197

tonyvanriet opened this issue Sep 30, 2019 · 5 comments · May be fixed by #325

Comments

@tonyvanriet
Copy link

We get a lot of mileage out of #183 when we want to be more precise about the lines we're ignoring. Thanks @mruoss.

Nonetheless, we just noticed that if we don't properly terminate a # coveralls-ignore-start, excoveralls will proceed to ignore the remainder of the file, which could hide a coverage problem below the point where we intended to stop the ignore.

In our case, we had some cases where we erroneously used coveralls-ignore-end instead of stop, and it has taken us quite a while to notice the mistake.

I don't know if/when I'll be able to make time to try to PR this, but I wanted to at least come here first to see if there was anything missing from my understanding, or if there was already discussion along these lines.

Thanks

@RKushnir
Copy link
Contributor

I think it's a great observation. And I would be happy to make a PR if I first get a decision on #301, because it's changing the same code.

@mruoss
Copy link
Contributor

mruoss commented Feb 19, 2023

That's neat! I also like what credo does: # credo:disable-for-lines:<count>. Could be adapted as well.

@RKushnir
Copy link
Contributor

RKushnir commented Mar 6, 2023

@tonyvanriet What would be the desired behavior?

  1. I think we should print a warning message.
  2. Should the exit code be 0 (success) or 1 (failure)?
  3. Should the remainder of the file be ignored or not?

Additionally, I think we could print a warning in case there is an unrecognized comment with prefix coveralls- or coveralls-ignore-. That would catch a mistake like coveralls-ignore-end.

@tonyvanriet
Copy link
Author

Thanks for picking this up @RKushnir.

I think a failure exit code would make sense and a message to indicate "unmatched ignore tag". If the parser can't find a valid being and end to the ignore block, I don't think it should ignore anything for the rest of the file. I think it makes sense to err on the side being conservative for a coverage tool like this.

@RKushnir
Copy link
Contributor

RKushnir commented Mar 5, 2024

So it's been exactly a year since I showed up here 😅 But recently I actually started working on this and I think I'm about half-way done coding. Before I proceed, I'd like to get some authoritative approval/feedback first, to avoid wasting time.

My solution is going to noticeably complicate the Ignore module, so I want to know if we find it worthwhile. The general idea is to use Stream.transform to process the coverage lines. When we see a line with # ..ignore-start, we start buffering the following lines into a temporary list. If we then see an # .. ignore-stop (happy path), we "flush" the buffer to the output with ignored coverage, mimicking the current behavior. If we reach the end of the file (or we see a new # .. ignore-start) before there is a closing # .. ignore-stop, we flush the buffer without ignoring. In other words, a start without a stop is discarded, and there is a warning log message.

The invalid cases which I'm handling and showing warnings:

  1. ignore-start but no ignore-stop until the end of file
  2. ignore-start then again ignore-start without ignore-stop in-between.
  3. ignore-stop without a corresponding ignore-start before it
  4. redundant ignore-next-line inside of a start/stop block
  5. ignore-start that is in the next line after ignore-next-line
  6. double ignore-next-line
  7. ignore-next-line as the last line of code

The first 2 cases are important, the others are just an easy bonus and can be skipped if needed for simplicity.

Please, guide me @parroty . I can also create a separate issue to discuss the implementation or publish my half-baked PR already.

RKushnir added a commit to RKushnir/excoveralls that referenced this issue Mar 27, 2024
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.

3 participants