Code coverage standard

[jdkandersson]

Explicitly stating what shouldn't be covered with a reason and the rest of the code should have 100% coverage

[merkata]

Isn't 100% a bit too much to begin with? There is benefit to this no doubt - with every step towards 100% you understand more of the code and how to test it, and having coverage means easier refactoring and adding new features. The cost here is time spent on this and not on something else, so it will be a trade off at some point when other priorities come. If we go at hard 100% and not something like hard 85% and everything above is more than welcome if there are no other commitments, we would need to defend this, as getting to 100% could take you longer and longer on the last percentages imo. There was also a discussion about that standard with @mthaddon .

[jdkandersson]

@merkata the idea isn't to mandate 100% coverage across the entire code base, just configure the tool to state what we want to be covered and then go for 100% of that. So, for example, any file or function we don't want to cover, we exclude from the coverage checking. This means that the tool tells you "of what we want to cover as a team, we have 100%". If there is no value or we prioritise something else, easy, just exclude the areas that are wasteful or low priority from the coverage checks. This makes it much easier to spot regressions in coverage or mistakes (e.g., I thought I covered this if statement, but it slipped through), which can be difficult to spot with an overall target of less than 100%. Does that help?

[merkata]

Alright, I think I get it now, thanks @jdkandersson ! Then my only question would be how is the test coverage represented - what will a consumer of the chart see - 100% or less? I'm not familiar with the tooling and the visual end result, that is why I'm asking. My only concern here is that if we present 100% test coverage that covers "our 100%" from a codebase, the total coverage of the codebase might be around 60-70%, but we give a different number because this is "what we want to cover as a team" - it sounds like false advertising. To quote Anchormen - "60% percent of the time, works all the time." :)

[jdkandersson]

@merkata that is a good question, the tooling will say that the code coverage is 100% and it shows what has been excluded. The false advertising problem is at least partially addressed by that it will be obvious what part of the code isn't covered because, either in the code or in the configuration, the files will be excluded. This means that, during any PR, there can be a debate about whether we should actually cover something that currently has been excluded, such as a function that is a likely source of bugs or critical to operations.

[mthaddon]

Do you have an example output you could share of something where you're excluding certain lines? I feel like if things are reporting 100% there might be less incentive for someone to increase test coverage over time, but that could be because I'm not familiar with how this would actually look.

[jdkandersson]

This is what it looks like for the jenkins-agent charm from the coverage html report. The overall report shows the number of lines excluded:

And this is what the lines look like:

In this case the way to exclude the if statement was using the # pragma: no cover comment next to the if statement. The incentive could be that this comment no longer appears next to a function.

[mthaddon]

I think what's hard to understand from this output is whether something is excluded because it's impossible (sure, nothing is impossible :)) to test, or because we just haven't got around to it yet. It also doesn't include in the test output (unless this is redacted) the missing lines, which would otherwise show and to me give more of an incentive for someone to review and fix. I personally prefer targeting 85% across the board, but aiming for higher on a per project basis, and aiming to not reduce coverage whenever we land a new change (as we do currently for indico).

If others disagree I don't want to stand in the way, just offering my opinion here.

[jdkandersson]

I'm happy to go with the 85%, what can help with why something was excluded is to include a reason near the comment. I did some research yesterday whether there was a way to show a coverage percentage ignoring exclusions but couldn't find anything yet. Maybe one modification is the maximum of the current coverage and 85% so that as we increase from 85% we discourage regression?

[varshigupta12]

I like the idea of keeping it at a maximum of current coverage and 85%. In case a project is at higher than 85% coverage, the introduction of new code shouldn't reduce it.