Use the Original percent_covered

[keunhyung-chung]

We can use the original percent_covered instead of recalculating it.

[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

This PR does not seem to contain any modification to coverable code.

[ewjoachim]

Hey, thanks for the contribution. Can you expand a bit on why this is something we want to do ?

[ewjoachim]

/e2e

[keunhyung-chung]

@ewjoachim Thanks for asking.
We need this fix because there was an inconsistency in the coverage measurement.

For example,

python-coverage-comment-action/coverage_comment/coverage.py

Lines 216 to 218 in ab22836

	percent_covered=compute_coverage(
	data["totals"]["covered_lines"],
	data["totals"]["num_statements"],

calculates the coverage without branch measurement.

As the result, even if we calculate the coverage using branch measurement (e.g., 60%), the coverage ratio in the badge will be shown as the coverage without branch measurement (e.g., 75%).

[ewjoachim]

If we add it back, it might be better to add it back fully with documentation, see #336 . We also may want to adapt the markdown table to display proper values.

Also, if possible, I'd rather we do the calculation ourselves every time rather than sometimes doing it and sometimes depending on the data having done it for us. I'm not saying that it's currently the case, and we might fix it, but if we're going towards one side of this question, I'd rather it's the "side I consider the most correct as of now": computing the coverage rate ourselves, potentially taking into account branching to get the correct value.

(I also have #347 in mind and I think it may be interesting to avoid expanding our tight coupling with Coverage just yet)

[keunhyung-chung]

If we add it back, it might be better to add it back fully with documentation, see #336 . We also may want to adapt the markdown table to display proper values.

Also, if possible, I'd rather we do the calculation ourselves every time rather than sometimes doing it and sometimes depending on the data having done it for us. I'm not saying that it's currently the case, and we might fix it, but if we're going towards one side of this question, I'd rather it's the "side I consider the most correct as of now": computing the coverage rate ourselves, potentially taking into account branching to get the correct value.

(I also have #347 in mind and I think it may be interesting to avoid expanding our tight coupling with Coverage just yet)

Hello @ewjoachim! Can you tell me what action I should take in this PR?

[ewjoachim]

Well, it's mainly that discussing the issue before jumping into the code might have been an easier way to set everyone's expectations straight. I think it might be more work than what was here so far. Of course if you want to tackle it, it's very nice, but I also understand if you'd rather not.

From the top of my head, here's how I imagine the things to look like:

1. Understand how calculation of coverage branch actually works in detail (feel free to write a bit in the contributing doc)
2. Evaluate whether we have all the data necessary to compute the branch rate
3. Change the compute_coverage function to take necessary additional parameters to output the right value (with 2 decimal digits, using decimal. Note that in your PR, you used rounding. It's probably not explicit enough but we try to truncate rather than round. 99.99% of coverage is not 100%), the function should take a boolean indicating whether we want branch coverage or normal coverage. This also includes modifying existing unit tests of said function, and adding all necessary new test cases.
4. Update all uses of compute_coverage to pass the new parameters. The boolean mentioned above should come from the project settings
5. Add the new project setting in settings.py as well as in the action manifest and in the README documentation. When writing the documentation, ensure that the term "branch" coverage is defined in a very explicit way, considering that throughout the whole project, "branch" is used in the context of a git/GitHub branch. Let's ensure we don't make this any more confusing than it already is.

Also, when you're done, please consider rebasing your commits into separate parts of work to ease commit-by-commit reviewing. That said, this part is optional.

[keunhyung-chung]

Well, it's mainly that discussing the issue before jumping into the code might have been an easier way to set everyone's expectations straight. I think it might be more work than what was here so far. Of course if you want to tackle it, it's very nice, but I also understand if you'd rather not.

From the top of my head, here's how I imagine the things to look like:

1. Understand how calculation of coverage branch actually works in detail (feel free to write a bit in the contributing doc)
2. Evaluate whether we have all the data necessary to compute the branch rate
3. Change the compute_coverage function to take necessary additional parameters to output the right value (with 2 decimal digits, using decimal. Note that in your PR, you used rounding. It's probably not explicit enough but we try to truncate rather than round. 99.99% of coverage is not 100%), the function should take a boolean indicating whether we want branch coverage or normal coverage. This also includes modifying existing unit tests of said function, and adding all necessary new test cases.
4. Update all uses of compute_coverage to pass the new parameters. The boolean mentioned above should come from the project settings
5. Add the new project setting in settings.py as well as in the action manifest and in the README documentation. When writing the documentation, ensure that the term "branch" coverage is defined in a very explicit way, considering that throughout the whole project, "branch" is used in the context of a git/GitHub branch. Let's ensure we don't make this any more confusing than it already is.

Also, when you're done, please consider rebasing your commits into separate parts of work to ease commit-by-commit reviewing. That said, this part is optional.

@ewjoachim Hello, thanks for the detailed guidelines. There is one thing I would like to ask before I start doing this. If I follow your guide, the coverage rate is calculated by compute_coverage function of this py-cov-action GitHub Actions. But I have a question about this part.

We run a test before doing this py-cov-action. For example, we run pytest --cov src and with this command we have already obtained the coverage rate. For example,

$ pytest --cov src
---------- coverage: platform linux, python 3.11.7-final-0 -----------
Name                                                         Stmts   Miss Branch BrPart   Cover   Missing
---------------------------------------------------------------------------------------------------------
src/.../__init__.py                                             3      0      0      0 100.00%
src/.../__main__.py                                             7      7      2      0   0.00%   2-14
...
---------------------------------------------------------------------------------------------------------
TOTAL                                                         2180   1154    886     75  42.04%

As shown above, we have already obtained a coverage rate of 42.04%, and this value is stored in data["totals"]["percent_covered"] as I have already used in this PR.

Before I proceed, I'd like to ask why we can't just use data["totals"]["percent_covered"] out of the box.

[ewjoachim]

Issues like #370 and #347 are making me wary of adding more tight coupling between this lib and Coverage.

I try to be mindful of the choices we make consuming (or not consuming) features from our dependencies so that we stay in control and when we want to make a change, we somewhat limit the risk.

It's pretty obvious that whatever tool we'll use, we'll need to have the raw data. But will the tool also provide the totals ? I don't know. If we compute the totals ourselves, this is not a question we need to ask.

[keunhyung-chung]

Issues like #370 and #347 are making me wary of adding more tight coupling between this lib and Coverage.

I try to be mindful of the choices we make consuming (or not consuming) features from our dependencies so that we stay in control and when we want to make a change, we somewhat limit the risk.

It's pretty obvious that whatever tool we'll use, we'll need to have the raw data. But will the tool also provide the totals ? I don't know. If we compute the totals ourselves, this is not a question we need to ask.

@ewjoachim I agree with your point. Unfortunately, I am afraid that I cannot continue what you requested in #361 (comment). 😢

Please close this PR if you want.

[ewjoachim]

I understand, and I'm sorry too. Thanks for your contribution anyways. Please note that it's not fun from my side either to decide whether contributions are accepted or not, especially when the answer isn't positive, and especially in cases like this where it really is a question of gut feeling, and not of quality or anything.

I think I'll close the PR, but I'll open an issue for properly dealing with branch coverage.

(If the situation evolves, I might reopen and merge at some point)