Branch coverage

[ferdnyc]

This PR builds on @ewjoachim 's WIP #413, completing the changes and updating the relevant tests so that all tests pass successfully when run locally with poetry run pytest. The pre-commit hooks were also run on all commits, and any ruff changes incorporated. I have not, however, run the E2E tests; I figured that could wait until after the PR was opened.

The PR branch contains four commits, the first being @ewjoachim's WIP changes to coverage_comment.coverage. The second commit is my own changes to complete the branch-coverage support.

A single new unit test, test_compute_coverage_with_branches, is added to tests/unit/test_coverage.py in the third commit, to exercise the updated coverage_comment.coverage.compute_coverage functionality in cases where (non-zero) branch coverage values are included.

The fourth commit adjusts the conftest.py fixtures and the various tests in test_template.py to both supply and expect coverage data computed accounting for branch coverage, raising the actual coverage for the fixture(s) data from 60% to 53.84% 62.5%.

I'll add a bit of commentary on specific changes as review comments, so they're easier to discuss in the proper context.

[github-actions]

End-to-end public repo

Admin commands cheatsheet:

• /e2e (in approved PR review body): Trigger end-to-end tests on external contributions
• /invite (in comment): Invite the author & admins to the end-to-end private repo

[github-actions]

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
coverage_comment
coverage.py
Project Total

This report was generated by python-coverage-comment-action

[ewjoachim]

I have not, however, run the E2E tests; I figured that could wait until after the PR was opened.

You're right. It's extremely annoying to run e2e tests for external contributors, and it's perfectly ok to wait for maintainers to launch them. Sadly, we need to use 2 github accounts to run those, but, at least, they provide a real-world test that's much better to trust the changes.

[ewjoachim]

I'll dig a bit but I'm surprised we don't need to update the template.

For now, very nice start.

Feel free to say if you have some feedback on the codebase & contribution process, especially if you saw things that were improvable, confusing, concerning etc. Of course, this applies to the next steps of your contribution too.

[ewjoachim]

/e2e

(approving just to run the e2e tests for now)

[ferdnyc]

branch coverage + diff coverage = 🤯

That was precisely my reaction as well, at which point I went: "Maybe I'm overthinking it. Even with branches, lines in the diff are either covered or not, right?" I don't actually know if that IS right, but I figured I'd run with it and see how it goes. #YOLO

[ewjoachim]

The e2e tests ran here producing:

• a public repo
• and a private repo

[ewjoachim]

Do we need to update some of the numbers & wording in the template ?

[ewjoachim]

So just to follow:

• we may want to adjust the exact value to get a coverage rate that doesn't have infinite decimals (Branch coverage #466 (review))
• Add a unit test there
• Regarding the comment template: the wording may be very slightly off because it says "statement" where it would actually now be "statement or branch". I'm not sure we want to overcomplicate things, so it's ok if we leave it this way, but if you want to update it, it's welcome :)

[ferdnyc]

@ewjoachim

Do we need to update some of the numbers & wording in the template ?

That's a good question — there was one spot where, in the test_template code, I ended up changing an expected statement like "the coverage is 60% (6/10)" to "the coverage is 53.84% (6/10)" which struck me as more than a little suspicious. So it's probably worth figuring out a better way to express the values there.

Regarding other changes, obviously things like the unit test addition and any template changes would be new commits, but for adjustments to existing changes (like the @dataclass(kw_only=True) tweak or the cast removal), do you prefer separate additional commits for reviewability, or rewritten and force-pushed PR commits for a clean history?

[ferdnyc]

@ewjoachim Oh, also, can I get an invite to the private e2e repo?

[ewjoachim]

@ewjoachim Oh, also, can I get an invite to the private e2e repo?

Oh, interesting ! It should have been done automatically and it didn't
https://github.com/py-cov-action/python-coverage-comment-action/actions/runs/10646793964/job/29514024275?pr=466

Looks like it's because true == 'true' evaluates to false in GHA lingo

Let's fix it (in another PR)

[ewjoachim]

do you prefer separate additional commits for reviewability, or rewritten and force-pushed PR commits for a clean history?

TL;DR: as you wish, I don't have strong opinions. But I appreciate discussing it and get to know what you prefer.

Glad you asked !
It's perfectly ok to rebase your commits for the sake of commit cleanliness, but it's true that it makes reviewing slightly harder. Nowadays, though, GitHub tries to help you a bit but it's not always ideal.
It's also perfectly ok if the commit history resembles what really happened in the contribution. I haven't spent a lot of time reading the commit history on this project, so I'm not 100% sure this is the best way to spend precious contributing time, but you seem interested :)

It sounds like you might be introduced to the wonderful git commit --fixup (if you don't happen to know already).

When you wish to modify a commit but don't want to hinder reviewability, make the changes that would supposedly go in a commit and commit them with git commit --fixup <hash of that commit>. This will create a commit named !fixup <name of the commit you targeted. Push this for review.

When everything has been reviewed and you're ready to merge, use git rebase --autosquash (compatible with --interactive) and those commits will be automatically moved in fixup mode, right after the commit they target (in other words, they will be merged into this commit) and voilà :) (of course, you'll need to push force for this.

Of course, there are also cons to this approach (some of which are shared with other solutions):

• it requires more manual operations
• when I'm ready to merge, either I'll need to rebase your commits, or wait for you to do so
• The risk exists that I'll forget and commit the fixup commits which is a dent in the history
• Also, there's a theoretical risk that you'll have a conflict when rebasing, and that the conflict is not handled properly leading to issues, especially though I'll only do a light review on the final commits
• And of course, there's a risk that you're a malicious actor and you'll embed malicious content when you rebase your fixups, hoping that I won't see it as I don't have a reason to suspect the post-rebase review will be relevant.

Note

I mentioned force pushing, and I'm contractually required to mention using --force-with-lease instead or --force. If you already know about it, feel free to skip this note. Otherwise, here you are. --force-with-lease works the same as --force as long as no one else pushed to your branch. In the event that someone pushed to your branch: either you've already fetched their commits (they're already in your origin/<branch> ref) and git considers you know about them and knowingly wishes to overwrite them, or there are commits on the branch that you don't know about (not yet in your origin/<branch>) and git won't let you push force (you need to fetch first, and possibly deal with those commits if you want). In other words, when you push force after a rebase, --force-with-lease is always the same or better than --force

[ewjoachim]

Let's retry after the fix.

/invite

[github-actions]

End-to-end private repo

[ewjoachim]

It worked https://github.com/py-cov-action/python-coverage-comment-action/actions/runs/10657091714/job/29536514522?pr=466

You should have received the invitation :)

[ferdnyc]

Finally had a chance to return to this. Not complete yet, but changes so far:

• I rewrote the commit updating the test data, so that the new test data works out to a cleaner (and more rational) 62.5% coverage.
• Added new commits to:
  • Make the complex dataclasses kw_only
  • Dump the casting in compute_coverage
  • Fix the defaulting of branch stats in _make_coverage_info()

The last one still needs that unit test, and I want to take another look at the textual descriptions in the template to see if they can better accommodate branch stats when available.

(Edit: Unit tests added.)

[ewjoachim]

Seems awesome so far :)

[ferdnyc]

@ewjoachim Could you please fire off another run of the end-to-end tests? I want to get a look at the current comment templates' output in rendered form. Reading it in encoded form in string variables inside the tests is... less-than-enlightening.

[ewjoachim]

Wooops totally missed that !

[ewjoachim]

/e2e

[ferdnyc]

@ewjoachim No worries, I don't know why I asked for it — the current e2e tests don't really show any of the branch coverage features, because they don't include any code changes (just coverage changes).

I was thinking of submitting a (separate) PR that adds an actual code modification to the e2e tests, so that we could get a look at the diff coverage reporting as well. That's the part, if anything, that I think might need adjusting with branch coverage enabled.

(I figured I'd drop the test-ed function to just contain

def f(a="", b="", c="", d=""):
      elements = []
      if a:
          elements.append(a)
      if b:
          elements.append(b)
      """__ADD_CODE_HERE__"""
      return "-".join(elements)

...And then have the e2e test function insert the if c: and if d: blocks. (Or maybe even if c: ... elif d: which might make for better branch coverage testing.)

WDYT?

[ewjoachim]

Good idea :)

[ewjoachim]

Hey what would you say that:

• we merge as-is
• I'll be delighted if you want to continue iterate on this

Deal ?

[ferdnyc]

@ewjoachim Sure, that works for me. There's nothing that can't be cleaned up in a future PR.