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

[tonyvanriet]

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]

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]

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

[RKushnir]

@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]

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]

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.