This is a test PR

[ewjoachim]

This PR introduces bogus code, but is here as a draft for collaborating on improving the coverage comment, related to #189

[github-actions]

End-to-end public repo

[ewjoachim]

Coverage report

Coverage details

File	Stmts	Miss	Cover	Missing
coverage_comment/
files.py	64	2	98%	74 python-coverage-comment-action/coverage_comment/files.py Line 74 in 4e3ee3a print("bar")
main.py	44	3	97%	65–66, 79 python-coverage-comment-action/coverage_comment/main.py Lines 65 to 66 in 4e3ee3a print("yay") print("foo") python-coverage-comment-action/coverage_comment/main.py Line 79 in 4e3ee3a print("yay")
Project Total	1055	739	99%
Added by PR	12	10	83%

_{This report was generated by python-coverage-comment-action}

[github-actions]

Coverage report

The coverage rate went from 100% to 100% ➡️
The branch rate is 100%.

100% of new lines are covered.

Diff Coverage details (click to unfold)

coverage_comment/main.py

100% of new lines are covered (100% of the complete file).

coverage_comment/files.py

100% of new lines are covered (100% of the complete file).

[ewjoachim]

I don't have any other idea for my comment above. I believe we're good from my point of view. Any other change you'd like to make @kieferro ?

[kieferro]

I don't have any other idea for my comment above. I believe we're good from my point of view. Any other change you'd like to make @kieferro ?

That looks pretty good. Much clearer and more informative. However, I still have two things:

• Is coverage_comment/ in this case supposed to be understood as COVERAGE_PATH or do we want to combine files in one folder in every case? For the latter case: Which system do we use for that? For example, what do we do if there are relevant files on several different levels of the file tree?
• I have the feeling that the distinction between coverage data and DiffCoverage data is not entirely clear in the data points on the files. I assume, for example, the missing lines only refer to the code added by the PR? Perhaps we could then make this a bit clearer or, for example, as before, show both the coverage of the whole file and coverage of the added lines.

[ewjoachim]

Is coverage_comment/ in this case supposed to be understood as COVERAGE_PATH or do we want to combine files in one folder in every case? For the latter case: Which system do we use for that? For example, what do we do if there are relevant files on several different levels of the file tree?

What I had in mind was to iterate on all the files that appeared on the coverage report, group them by "full folder path" and then display all the files belonging in the same folder together. No nesting. So for example:

• a/b/
  • c.py
  • d.py
• a/b/e/
  • f.py
• g/
  • h.py

I have the feeling that the distinction between coverage data and DiffCoverage data is not entirely clear in the data points on the files. I assume, for example, the missing lines only refer to the code added by the PR? Perhaps we could then make this a bit clearer or, for example, as before, show both the coverage of the whole file and coverage of the added lines.

Excellent point. I guess it would make sense to have the new coverage (or even the evolution in coverage) for each file, but also the diff coverage, and those are 2 different things.
Maybe instead of Stmts | Miss | Cover | Missing, we should have Previous file coverage rate | Current file coverage rate | Coverage rate of added lines | Lines missing coverage | Added lines missing coverage. (this would mean we'd need to store much more precise data in the json file)

• "Lines missing coverage" will have to link to the branch and not the PR.
• "Added lines missing coverage" can link to the PR diff

Is it too much ? I'm afraid it might be overwhelming.

[ewjoachim]

If we have previous coverage

---

Coverage report

Coverage details

Coverage Details This table shows all the files where coverage has changed
File	Previous coverage rate	Current coverage rate	Coverage rate of added lines	Lines missing coverage (Lines added by this PR in bold)
coverage_comment/
files.py	96%64/66	65/66 98% 📈	2/3 66%	66, 74 python-coverage-comment-action/coverage_comment/files.py Line 66 in 4e3ee3a color = badge.get_badge_color( python-coverage-comment-action/coverage_comment/files.py Line 74 in 4e3ee3a print("bar")
main.py	64/66 96%	65/66 98% 📈	2/3 66%	65–66, 79 python-coverage-comment-action/coverage_comment/main.py Lines 65 to 66 in 4e3ee3a print("yay") print("foo") python-coverage-comment-action/coverage_comment/main.py Line 79 in 4e3ee3a print("yay")
Project Total	96%490/500	494/500 98% 📈	4/6 66%

_{This report was generated by python-coverage-comment-action}

---

If we don't have previous coverage

---

Coverage report

Coverage details

Coverage Details This table shows all the files where coverage has changed
File	Current coverage rate	Coverage rate of added lines	Lines missing coverage (Lines added by this PR in bold)
coverage_comment/
files.py	65/66 98% 📈	2/3 66%	66, 74 python-coverage-comment-action/coverage_comment/files.py Line 66 in 4e3ee3a color = badge.get_badge_color( python-coverage-comment-action/coverage_comment/files.py Line 74 in 4e3ee3a print("bar")
main.py	65/66 98% 📈	2/3 66%	65–66, 79 python-coverage-comment-action/coverage_comment/main.py Lines 65 to 66 in 4e3ee3a print("yay") print("foo") python-coverage-comment-action/coverage_comment/main.py Line 79 in 4e3ee3a print("yay")
Project Total	494/500 98% 📈	4/6 66%

[!NOTE]
No coverage data of the default branch was found for comparison. A possible reason for this is that the coverage action has not yet run after a push event and the data is therefore not yet initialized.

_{This report was generated by python-coverage-comment-action}

[ewjoachim]

Note

I don't know which of those is better:

96%64/66
vs
_64/66
96%

[kieferro]

group them by "full folder path" and then display all the files belonging in the same folder together. No nesting.

Yes , sounds good.

[kieferro]

I also like the new proposal, but I found the old one a bit tidier and easier to understand at first glance and actually I don't think it needs that many changes to the structure. I would just add a delta for each value. In order to highlight this in colour, we would have to work with a relatively large number of badges, because Markdown unfortunately doesn't support this natively, but otherwise why not just modify the original idea a little bit (The numerical values don't make sense, but you get the idea):

---

Coverage report

See where and how coverage changed

File	Stmts	Miss	Cover	Cover (new lines)	Missing
coverage_comment/
files.py					74 python-coverage-comment-action/coverage_comment/files.py Line 74 in 4e3ee3a print("bar")
main.py					65–66, 79 python-coverage-comment-action/coverage_comment/main.py Lines 65 to 66 in 4e3ee3a print("yay") print("foo") python-coverage-comment-action/coverage_comment/main.py Line 79 in 4e3ee3a print("yay")
Project Total

_{This report was generated by python-coverage-comment-action}

---

Also, I'm not quite sure if it makes sense to link all the missing lines. I can imagine that there are projects that do not yet have much coverage, which means that the missing lines that are actually relevant for PR are lost in the mass of missing lines. That's why I wonder if we shouldn't just focus on those?

[ewjoachim]

Also, I'm not quite sure if it makes sense to link all the missing lines

Just to make it clear: not all the missing lines, just the ones inside files where coverage has changed for the PR. Reasoning is that sometimes, coverage changed somewhere because of a change elsewhere. For example, you want to move a test, and delete it but forget to re-add it elsewhere, the tested code lost coverage but it's not part of the diff, it's still linked to the PR.

I like the idea of badges, I note that GitHub adapts the badge size to the available size, making some quite small:

(Not saying it's a problem we need to solve. We can be ok leaving it as-is)

There's always a small ambiguity when saying 88% (+10%) because it's not clear if 10% applies to 88% (so it was 80% before and we added 10% of 80% which is 8 percentage points leading to 88%) or to the underlying value (so it was 78% before). But it's not a big ambiguity, I think it can be ok it we choose to keep it.

In my example, I've removed the branch coverage rate because I'm not sure it's more useful than confusing. We're in the context of a PR, and "branch coverage" could be easily understood as a synonym for "PR coverage" though it's completely different.

We're not displaying the number of new line added/covered, is this by design ? We're only displaying the percentage.

I had added a header row to the table, do you think it's worth keeping ?

[kieferro]

Not all the missing lines, just the ones inside files where coverage has changed for the PR. Reasoning is that sometimes, coverage changed somewhere because of a change elsewhere.

Good point, I hadn't thought about that but, of course, it makes a lot of sense to do it that way.

There's always a small ambiguity when saying 88% (+10%) because it's not clear if 10% applies to 88%

Yes, I have thought about that too. I also thought about whether it would make more sense to write (-0.21pp) (pp meaning percentage points), but I think that would lead to even more confusion. Maybe we can add a footnote or a little explanation.

In my example, I've removed the branch coverage rate because I'm not sure it's more useful than confusing. We're in the context of a PR, and "branch coverage" could be easily understood as a synonym for "PR coverage" though it's completely different.

Ah sorry, I had just accidentally copied those from the original example. I'm completely on board with removing those 👍

We're not displaying the number of new line added/covered, is this by design ? We're only displaying the percentage.

I'm not quite sure what you mean. For example files.py had 64 statements before and now has two more so 66 and it had 2 not covered statements and now has 3 not covered statements. Are you referring to the difference between statements and lines or what other information would be necessary in your opinion?

I had added a header row to the table, do you think it's worth keeping ?

I'm not sure, it takes up quite a lot of space. Why don't we move the information from it to the area that is clicked to expand:

Coverage details (Information about all the files where coverage has changed)

[ewjoachim]

I'm not quite sure what you mean. For example files.py had 64 statements before and now has two more so 66 and it had 2 not covered statements and now has 3 not covered statements. Are you referring to the difference between statements and lines or what other information would be necessary in your opinion?

Say files.py had 64 statements and now has 66, and has 50% of added lines covered. Without more information, you'd probably think you added 2 lines, one of which was covered. But maybe you actually rewrote the whole file and covered 33 lines, so we'd be missing 33 lines. I think the number of added lines may be important and is not deduced just by seeing how many lines there were before and there are now because you don't know how many were deleted.

The list of lines missing coverage may help get a sense of this but it's only showing missing lines.

[kieferro]

Ah okay, I see. I have edited my proposal and also added this information. I think that it also looks a bit more consistent now. Would you agree with the layout and styling then?

[ewjoachim]

I think we're good!

[ewjoachim]

I've changed a detail on the <details><summary>:

[kieferro]

I am currently working on the PR and I feel that it would be useful to make the selection of files that appear in the table a little different than it is now.
For example, if you delete a test case for a file, then if we go by DiffCoverage as before, the coverage loss in that file would not be shown.
So I think we need a different condition. The best thing I can think of would be to include all files where the number of statements, misses or both have changed. Because in all these files something has changed in the tested code or in its coverage. What do you think about that @ewjoachim?

[ewjoachim]

Yes, "listing all files where coverage has changed" was my current understanding of what we wanted to do so I guess we're aligned ?
(That said, maybe we should limit to, say, 50 or 100 files. Like: we'd show the 100 files with the biggest difference in coverage, ordered by name)

[ewjoachim]

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
codebase
code.py					12-14, 22
other.py
third.py
Project Total

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

[ewjoachim]

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
codebase
code.py					12-14, 22
other.py
third.py
Project Total

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

[ewjoachim]

Closed by #211