[Bug]: Wrong failure behavior when comparing with diff

[emmanuelsdias]

What happened?

Hello, first of all just wanted to say that I really like this project, it's one of the best coverage actions I've used so far!

I think the new-uncovered-statements-failure and coverage-reduction-failure behaviors are swapped. When using coverage-reduction-failure, even when overall coverage increases, my action fails if there's any Missing > 0 in the diff report.

According to pycobertura's docs on diff exit codes, we have the following exit codes:

• 2: the changes worsened the overall coverage
• 3: the changes introduced uncovered statements but the overall coverage is still better than before

As you can see, pycobertura's error exit code 3 indicates that overall coverage increased despite new uncovered lines. However, when we check this action's code, we see that coverage-reduction-failure checks for exit code 2, not 3.

coverage-action/action.yml

Line 372 in 4ee65a3

if [[ "${{ env.pycobertura_status }}" == "2" && "${{ inputs.coverage-reduction-failure }}" == "true" ]]

Meanwhile, the new-uncovered-statements-failure matches the worsened the overall coverage descripition, which associates any increase in uncovered line as failures.

coverage-action/action.yml

Line 378 in 4ee65a3

if [[ "${{ env.pycobertura_status }}" == "3" && "${{ inputs.new-uncovered-statements-failure }}" == "true" ]]

Relevant log output

No response

Code of Conduct

• I agree to follow this project's Code of Conduct.

Contribution Guidelines

• I agree to follow this project's Contribution Guidelines.

Security Policy

• I agree to follow this project's Security Policy.

[cicdguy]

Thanks for this! https://github.com/insightsengineering/coverage-action/releases/tag/v2.7.2 is now available with this fix

[emmanuelsdias]

Hello, I'm really sorry but it seems that my "fix" was actually wrong. I've misunderstood how pycobertura's codes work and what they mean by overall coverage getting better / worse. The previous association seems to have been mostly correct. Meanwhile, after my fix it got completely wrong.

pycobertura's exit codes are defined according to the get_exit_code function (here) and two methods, has_better_coverage (here) and has_all_changes_covered (here). Based on these functions, we can know the exact conditions in which each exit code is returned.

---

Exit code 0

• Docs description:
  • All changes are covered, no new uncovered statements have been introduced
• Conditions (based on implementation):
  • For all changed files, total number of uncovered lines reduced (Miss <= 0);
  • For all changed files, every new / changed statement is covered (Missing = []).

---

Exit code 2

• Docs description:
  • The changes worsened the overall coverage
• Conditions (based on implementation)::
  • Among changed files, at least one has total number of uncovered lines increased (Miss > 0);

When checking for the coverage-reduction-failure flag, we should probably use this exit code. Whenever coverage "reduces", i.e., there's an increase in uncovered lines for a file, the exit code is 2. However, I think that "coverage reduction" might be a bit confusing here, since it's more often associated with reduction of rate, not with increase of uncovered lines.

---

Exit code 3

• Docs description:
  • The changes introduced uncovered statements but the overall coverage is still better than before
• Conditions (based on implementation)::
  • For all changed files, total number of uncovered lines reduced (Miss <= 0);
  • Among changed files, at least one new / changed statement isn't covered (Missing != []).

As you can see, this exit code is only returned if overall coverage is "better" (i.e., decrease of uncovered lines) and there are new uncovered lines. Due to that, there's no way to actually check specifically for new uncovered lines (as suggested by the name of the flag new-uncovered-statements-failure and its description), because sometimes even though there are new uncovered lines, the exit code returned is 2, and not 3. It would be good to make it clear in the flag description that it's supposed to be used alongside the existing coverage-reduction-failure, i.e., it only fails if total uncovered lines decrease and new uncovered lines are introduced. If user wants to fail on both cases, they should set both flags as true.

---

Given the above, I can open a PR reverting the code changes and clarifying some of the flags usage, hopefully preventing similar mistaked to mine. IMO, uncovered-statements-increase-failure might be a better name for the existing coverage-reduction-failure flag, but fully changing it wouldn't be backwards compatible and I'm not sure what's your take here, what do you think?

I'll probably open a new issue with a feature request for a flag such as coverage-rate-reduction-failure, which only looks to the overall line rate and fails based on that. Although it's not perfect, it's a very common (and more forgiving) way of measuring coverage which would be useful to me and I believe for others too.