Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Branch coverage #466

Merged
merged 10 commits into from
Oct 5, 2024
Merged

Branch coverage #466

merged 10 commits into from
Oct 5, 2024

Conversation

ferdnyc
Copy link
Contributor

@ferdnyc ferdnyc commented Aug 31, 2024

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.

Copy link

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

Copy link

github-actions bot commented Aug 31, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  coverage_comment
  coverage.py
Project Total  

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

coverage_comment/coverage.py Outdated Show resolved Hide resolved
coverage_comment/coverage.py Outdated Show resolved Hide resolved
coverage_comment/coverage.py Show resolved Hide resolved
coverage_comment/coverage.py Outdated Show resolved Hide resolved
@ewjoachim
Copy link
Member

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.

coverage_comment/coverage.py Outdated Show resolved Hide resolved
Copy link
Member

@ewjoachim ewjoachim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

coverage_comment/coverage.py Outdated Show resolved Hide resolved
coverage_comment/coverage.py Show resolved Hide resolved
coverage_comment/coverage.py Outdated Show resolved Hide resolved
coverage_comment/coverage.py Show resolved Hide resolved
coverage_comment/coverage.py Outdated Show resolved Hide resolved
coverage_comment/coverage.py Outdated Show resolved Hide resolved
tests/conftest.py Show resolved Hide resolved
Copy link
Member

@ewjoachim ewjoachim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/e2e

(approving just to run the e2e tests for now)

@ewjoachim ewjoachim mentioned this pull request Aug 31, 2024
@ferdnyc
Copy link
Contributor Author

ferdnyc commented Aug 31, 2024

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

tests/conftest.py Outdated Show resolved Hide resolved
@ewjoachim
Copy link
Member

ewjoachim commented Aug 31, 2024

The e2e tests ran here producing:

@py-cov-action py-cov-action deleted a comment from github-actions bot Aug 31, 2024
@ewjoachim
Copy link
Member

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

@ewjoachim
Copy link
Member

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
Copy link
Contributor Author

ferdnyc commented Sep 1, 2024

@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
Copy link
Contributor Author

ferdnyc commented Sep 1, 2024

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

@ewjoachim
Copy link
Member

@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
Copy link
Member

ewjoachim commented Sep 1, 2024

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
Copy link
Member

Let's retry after the fix.

/invite

Copy link

github-actions bot commented Sep 1, 2024

End-to-end private repo

@ewjoachim
Copy link
Member

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
Copy link
Contributor Author

ferdnyc commented Sep 13, 2024

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.)

Copy link
Member

@ewjoachim ewjoachim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems awesome so far :)

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Sep 14, 2024

@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
Copy link
Member

Wooops totally missed that !

Copy link
Member

@ewjoachim ewjoachim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/e2e

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Sep 24, 2024

@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
Copy link
Member

Good idea :)

@ewjoachim
Copy link
Member

Hey what would you say that:

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

Deal ?

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Sep 27, 2024

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

@ewjoachim ewjoachim merged commit b2eb38d into py-cov-action:main Oct 5, 2024
5 checks passed
@ewjoachim
Copy link
Member

Sorry for the delay. I'm all over the place these days. Congratulations for you contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants