Feature request: maximum_coverage

[coorasse]

Howdy! I have a funny feature request and I am ready to open a Pull Request if anyone else is interested. I want to implement a maximum_coverage configuration. Once this value is set, if the coverage gets over this value, it raises an error. The behavior is similar to minimum_coverage but inverted. Even if this feature may sound funny it would be extremely useful for my case. I want to set this value to the same value of minimum_coverage. E.G.

SimpleCov.minimum_coverage 79.28
SimpleCov.maximum_coverage SimpleCov.minimum_coverage

by doing this I can always be sure that if a developer implements new tests and increase the code coverage, (s)he remembers to also increase the minimum coverage value.

I found myself in the situation where developers forget to update this value and increase it, allowing the next developer to drop the coverage since the minimum was too low.

Glad to hear your feedback. 😄

[AxelTheGerman]

Is SimpleCov.exact_coverage what you want in this case?

I get what you are saying - we have a similar scenario with a maximum warning counts from a static analysis tool. If you fix a bunch and don't update the value, the next dev comes along and can introduce new ones without even noticing.

However I think it's better to make it a habit to update this frequently instead of enforcing it. You'll end up breaking your main build or in merge hell if this is enforced on every PR

[PragTob]

:wave:

Hi there, I'm not sure I'd want this feature :sweat_smile:

There is SimpleCov.refuse_coverage_drop which admittedly doesn't work well when run across multiple machines. I think what you'd want is a feature like this from coveralls.

If implemented I think I'd reach for exact_coverage instead of setting a maximum coverage as @AxelTheGerman suggested. It's not for me to prescribe a workflow but I think very hard minimum coverage limits can be pretty hurting. Imagine Refactoring a well tested 10 line method to 5 lines. Suddenly my coverage drops (less lines covered) and I need to test untested code to make up for my awesome refactoring :cry:

@coorasse happy to be convinced otherwise but currently I don't really see it.

[coorasse]

@PragTob the situation you describe targets minimum_coverage, not the proposed maximum_coverage.

[coorasse]

@AxelTheGerman exact_coverage would work as well, 'cause in the situation I am describing we would always set minimum = maximum, making exact much more appropriated. Totally agree 👍 Do we have exact_coverage already? Interested in a PR?

[PragTob]

@coorasse well I'm talking not about maximum_coverage but about your intended use case of minimum_coverage + maximum_coverage or the proposed "exact_coverage". It'd still apply (if I remove 5 loc of tested code I have to drop the value) and happen there so I'm unsure if this is generally a good pattern. I'm hence a bit unsure if I'd wanna have it in simplecov and support the feature going forward. I think what you want to do is refuse_coverage_drop it just doesn't work across multiple different workers, or am I misunderstanding the problem?

(I also realize that the problem I describe also exists with refuse_coverage_drop)

[coorasse]

@colszowka would you be interested in a PR to introduce exact_coverage? If so, I'll proceed, otherwise I'll close this PR. Thanks

[PragTob]

@coorasse can you comment on my comment above? Is what you really want refuse_coverage_drop that works on multiple nodes or what do you want?

[coorasse]

I don't want to allow a coverage drop but also do not allow a coverage increase. It may sound weird: "why don't you want the code coverage to increase?". The answer is that if the code coverage increases but the value of minimum_coverage is not increased as well, the next developer can introduce non-tested code. If I am not clear enough I can try with an example. Thanks for taking the time @PragTob

[PragTob]

@coorasse I understood your original use case but I'm still not understanding how a correctly working refuse_coverage_drop doesn't cover it. From what I can see it does exactly what you'd want but without manual intervention, as in every PR MUST increase the baseline coverage. Coverage can never go down, only up.

So let's say it's at 80%, someone makes a PR with 79%, it fails. They change it to 81%. It passes, gets merged to dev and is now the new baseline. Now everyone afterwards would need to have test coverage >= 81% which if I understand correctly is exactly the behaviour you want.

[coorasse]

Ho PragTob, yes, this would work. Except that this forces me to run tests locally and commit a new last_run.json file and we usually don't do that, because it just blocks us, while the CI can run them for us. I also see that this is a very specific case for us, and I confirm you that it would solve our issue. 👍

[AxelTheGerman]

you probably don't want to commit last_run.json. What you want is your CI caching this file between runs. Either the CI has a built in cache - ideally per branch or falls back to base branch. Or you would want to store that in S3 or even something like https://jsonbox.io

I agree with PragTob that this feature doesn't seem to make sense for 99.9% of the users of this gem. Usually you want to enforce a minimum coverage or (more advanced) refuse coverage drop.

If you don't want developers to include untested code you would need to analyze the last_run.json and see if any of the new lines are not in there. I could easily make a PR with one new line, and simply write a test for an old uncovered line. This would pass all your restrictions while still doing exactly what you want to prevent.

Unless this is some weird compliance thing, I feel you might be focusing on the wrong thing with this.

[coorasse]

I think I have a good overview with the examples you provided. I still think a exact_coverage would be useful but I don't see much interest around so I can close this issue. Thanks for the time you took to look into it. 🙇

[PragTob]

@coorasse what would that use case be?

My intent behind pushing against it isn't that I don't want it, but rather that to my understanding what you really want is a refuse_coverage_drop - but one that works on modern CI systems - you know? :D

So rather than building a new features to deal with the inadequacies of an old feature I'd rather fix/extend the old feature.

@AxelTheGerman lined out the issue. simplecov just writes the file but in modern cloud based environments or even one with multiple workers/multiple projects it doesn't make much sense.

My current thought is to provide a way to tell simplecov where & when to store a new coverage reference version and how to retrieve it. Whether that's a gist, s3 or what not. (kind giving you the ability to specify your own block).

That's a new issue though, I believe that would work.

@coorasse since you brought that up, what's your CI environment like? Could we solve the problem like this for you?

[coorasse]

My CI environment is very simple, I don't distribute my tests, and I simply run all of them on one "job". At the end, I obtain the code coverage. I can achieve what I want with @AxelTheGerman solution. I could achieve it also with an exact_coverage parameter. Basically I want to solve this issue: "When the code coverage is increased and the minimum_coverage has not been increased accordingly: I want the CI to fail". The answer could be "set the exact_coverage". But it can also be what @AxelTheGerman proposed: "Set refuse_coverage_drop and store last_run.json file." I just find the second solution slightly more complicated, that's why I consider the first solution easier, but it solves the issue.