feedback icon indicating copy to clipboard operation
feedback copied to clipboard

Pull Request Comment Report

Open codecovdesign opened this issue 2 years ago β€’ 42 comments

Thanks for dropping by! πŸ‘‹

We'd love your feedback about the pull request comment, focused on the comment report details, layout, and information presented.

  • What do you like about the PR comment report?
  • What would you'd like to see improved?
  • What are challenges or frustrations you've experienced?
  • How do you use the comment today?
  • How could the report better help your workflow?

Any general thoughts you'd like to share!

We greatly appreciate your time and thoughts - looking forward to hearing from you ❀

Codecov team

This issue is intended to share and collect feedback about the tool. If you have support needs or questions, please see our support page.

codecovdesign avatar Aug 15 '23 20:08 codecovdesign

Our reports changed randomly and we are unsure how to get it back to how they were before.

New: image Old: image image

Any advice on how to get back the more detailed report?

bdaubry avatar Aug 31 '23 14:08 bdaubry

@bdaubry we're experimenting with a new PR comment format that we're rolling out incrementally across all users, collecting feedback as we go.

If you have any feedback on why you prefer the old PR comment to the new one, we would be glad to hear it. If you would like to revert to the old style of PR comment, you can place the following in your codecov.yml file at the root of the repository:

comment:
    layout: "newheader, diff, files, newfooter"

eliatcodecov avatar Sep 01 '23 14:09 eliatcodecov

Exactly what I needed, thank you!

If you have any feedback on why you prefer the old PR comment to the new one, we would be glad to hear it.

It's beneficial for us to see how much coverage has changed within specific files, as opposed to their total coverage. It allows us to see what might still need new test cases before a PR is merged, etc. Without the context of the previous coverage, we have to go into the builds to see the end result of the test suite, or run the tests locally with codecov.

bdaubry avatar Sep 05 '23 15:09 bdaubry

@bdaubry thanks for that feedback. I'm a PM on the Codecov team

Out of curiosity, is the total coverage (and file specific coverage trends) on the Codecov Web UI something you've considered looking at? If you feel it's not best suited to help you with planning tests to add etc, we'd love to learn why

rohan-at-sentry avatar Sep 19 '23 18:09 rohan-at-sentry

@bdaubry thanks for that feedback. I'm a PM on the Codecov team

Out of curiosity, is the total coverage (and file specific coverage trends) on the Codecov Web UI something you've considered looking at? If you feel it's not best suited to help you with planning tests to add etc, we'd love to learn why

It's nice that it is available there. However, we'll have PRs that are open for days to weeks, and having to go to Codecov every time would add an unnecessary step to our workflow. It was super handy having that information available in a comment on the PR that updated automatically, so anyone on our team could know exactly what files to target.

Unfortunately, the file specific trends appear to be gone from the PR comment, even following the earlier advice.

bdaubry avatar Sep 21 '23 13:09 bdaubry

@bdaubry Can you post a screenshot of your current PR comment? I think we can get you what you need

aj-codecov avatar Sep 25 '23 16:09 aj-codecov

The codecov PR comment is misleading when the changes are not covered by any tests at all. It says "All modified lines are covered by tests βœ…" for documentation changes for example.

ludofischer avatar Sep 28 '23 18:09 ludofischer

@bdaubry Can you post a screenshot of your current PR comment? I think we can get you what you need

Hello,

It appears between my last comment and now, our coverage deltas have returned. Current view: image

I think I'm good now, I appreciate you all looking into it!

bdaubry avatar Sep 28 '23 18:09 bdaubry

@ludofischer That's great feedback, thank you, we'll update that to be a little more relevant when the PR doesn't actually impact coverage at all - thoughts on verbiage like β€œThis change had no impact on code coverage.”?

@bdaubry Glad to here all is back to what you'd expect! Please let us know if you have any further issues or suggestions!

aj-codecov avatar Sep 29 '23 15:09 aj-codecov

@ludofischer That's great feedback, thank you, we'll update that to be a little more relevant when the PR doesn't actually impact coverage at all - thoughts on verbiage like β€œThis change had no impact on code coverage.”?

I think it is definitely better to say that code coverage did not change rather than saying that the lines are covered by tests. It's a bit tricky because the situations because completely uncovered code and impossible to cover changes like docs are fairly different cases.

ludofischer avatar Sep 29 '23 15:09 ludofischer

I really miss the diff view where I can see the converge change by lines, which gives a nice idea that is not conveyed by the percentage. I also like that it has colors (red/green) which makes it easy to scan. With PRs that touch many files a summary view is also very helpful.

Also when we update/improve only tests, coverage will increase but that will not be reflected on the changed files, because it's the source that is tested that will see the change. This increment can only be seen from the repo level coverage data.

justinchuby avatar Oct 13 '23 05:10 justinchuby

Hi @justinchuby - we really appreciate your feedback, if you'd like to flip the comment back, you can use the below configuration in your codecov.yml:

comment:
    layout: "newheader, diff, files, newfooter"

aj-codecov avatar Oct 13 '23 10:10 aj-codecov

I am getting a warning that a newly added function in a julia code base is not covered, although there is a docstring attached to it. This may be because there is a comment right before the docstring?

image

jlbosse avatar Oct 16 '23 11:10 jlbosse

Is this PR format simpler than it was before? Or does it just seem that way because this PR is at 100%? 🧐

If it's simpler I like it! :-)

P.S. There is a typo though ... double punctuation on "Let us know!."

Also this ticket I'm on has been in "Waiting for: Product Owner" for two weeks. 😭

Screenshot 2023-11-01 at 10 00 01 AM

chadwhitacre avatar Nov 01 '23 14:11 chadwhitacre

Right you are @chadwhitacre -- it is simpler, and there is double punctuation.

Fixing: https://github.com/codecov/worker/pull/164

jerrodcodecov avatar Nov 01 '23 14:11 jerrodcodecov

Thanks @jerrodcodecov! New simpler PR comment looks great! :-)

P.S. lolsob silly bot ... @hubertdeng123 can we convince the bot that @jerrodcodecov is not not a product owner here, and that I am not?

chadwhitacre avatar Nov 01 '23 14:11 chadwhitacre

The comments have changed recently, and I'm struggling to get back to a similar view as before.

It seems that the diff now appears in a collapsed section, which makes it too easy to ignore and not notice any issues in coverage.

Ultimately, the one thing I need to know is whether any previously covered lines are no longer covered, or newly added lines are missing coverage (including partials). If so, then I'll just click the link to the full report to see the exact lines in the diff view (and decide if they need to be covered or not).

The diff part shows this pretty clearly, with red/green highlights at a glance. But, now it is hidden under a collapsible section it is too easy to forget about (I thought that maybe the condensed_ versions were collapsed or something, but seems they all are).

Dreamsorcerer avatar Nov 25 '23 20:11 Dreamsorcerer

@Dreamsorcerer, thanks for the feedback! Could you share a screenshot of the report you're seeing? We want to make sure this isn't a format issue or previous version.

I need to know is whether any previously covered lines are no longer covered, or newly added lines are missing coverage (including partials). If so, then I'll just click the link to the full report

Our latest report format (below) displays any missed lines + related file (or confirms when there are none missing βœ… ) right at the top for immediate visibility. Is this the format your seeing and referring to? If not, is this a bit closer to your preference? Would love your thoughts! πŸ™

image

codecovdesign avatar Nov 27 '23 14:11 codecovdesign

I'm afraid that only covers direct changes in the patch: image

I've updated the section in our docs with an example from a recent PR, which shows exactly how we use codecov and why this is so important (the tool itself, and being able to see the diff at a glance): https://aiohttp--7916.org.readthedocs.build/en/7916/contributing.html#tests-coverage

If indirect changes are included in that summary, then it's probably fine. But, an "all green" status or something to indicate that no coverage issues have occurred would make me feel more comfortable when there are no files to display.

Dreamsorcerer avatar Nov 27 '23 17:11 Dreamsorcerer

Thank you @Dreamsorcerer! This is really helpful πŸ™

only covers direct changes in the patch:

That is true for now. I created an issue to capture the problem and review further with the team: https://github.com/codecov/engineering-team/issues/865

an "all green" status or something to indicate that no coverage issues

When all lines are covered, the comment summary should read "All modified and coverable lines are covered by tests βœ… " (example). Is this similar to how you were describing?

codecovdesign avatar Nov 28 '23 19:11 codecovdesign

When all lines are covered, the comment summary should read "All modified and coverable lines are covered by tests βœ… " (example). Is this similar to how you were describing?

It's possible that I just filter it out mentally at the moment, as the information is incorrect due to not including indirect changes. I guess I'll review again when all changes are included. But, certainly the strong red/green highlighting in the diff component is very clear at a glance, so that's my baseline.

Dreamsorcerer avatar Nov 29 '23 23:11 Dreamsorcerer

I'm not sure if this is a bug on your side or a config issue on my side, but I can't see the missing lines when I click the missing lines on a PR: https://github.com/maplibre/maplibre-gl-js/pull/3431#issuecomment-1835473074

Generally speaking, the PR comment is a lot cleaner and focused, and the missing lines is a good summary.

I'm not sure the report is up to date though as there was a passing build today, but the coverage comment is from yesterday.

If there was an option to open details in the PR comment to see some red and green lines on the relevant files (if the number of missing lines is small) it might improve productivity, i.e. similar to opening the details below the table - open details for every file to show which lines are missing.

HarelM avatar Dec 06 '23 20:12 HarelM

I'm not sure if this is a bug on your side or a config issue on my side, but I can't see the missing lines when I click the missing lines on a PR:

If you look at the URLs, they all just open the page for the PR. Seems that currently you'll still need to scan through the diff to find the missed lines.

I'm not sure the report is up to date though as there was a passing build today, but the coverage comment is from yesterday.

By default the comment is updated in-place. I don't think it's always obvious enough to tell if it's up-to-date or not, but you can compare the commit mentioned in the comment to see if it's referring to the latest commit pushed or not.

Dreamsorcerer avatar Dec 07 '23 11:12 Dreamsorcerer

If you look at the URLs, they all just open the page for the PR. Seems that currently you'll still need to scan through the diff to find the missed lines.

I'm sorry to be blunt here, but that doesn't make sense from a user experience point of view...

By default the comment is updated in-place.

I looked at the commit update time vs the last successful run time and they are not close, so I believe it wasn't updated correctly with the latest results, but I might be wrong...

HarelM avatar Dec 07 '23 12:12 HarelM

I'm sorry to be blunt here, but that doesn't make sense from a user experience point of view...

I'm just another user. :P Just pointing out the current behaviour. Maybe there's improvements on the roadmap.

I looked at the commit update time

I meant the actual commit ID. From one of my PRs: image

The comment mentions c473d40, and the last commit on the Github timeline shown after that comment is the same commit. So, I can tell it has been updated. Occassionally it fails to update, in which case looking at the CI run I can usually find an error message in the codecov upload step (but, it doesn't fail the CI when the upload errors).

Dreamsorcerer avatar Dec 07 '23 12:12 Dreamsorcerer

I don't know if the codecov comments get notified by git pushes, or only get notified when an upload happens. But, if it's the former, then it'd definitely be useful for the comment to get updated immediately with ** OUTDATED ** or similar at the top of the comment.

Dreamsorcerer avatar Dec 07 '23 12:12 Dreamsorcerer

πŸ‘‹ Also, the Edited word is a dropdown history of edits image

I'll leave all the design feedback for @codecovdesign to handle :D

drazisil-codecov avatar Dec 07 '23 13:12 drazisil-codecov

Although ...

I don't know if the codecov comments get notified by git pushes, or only get notified when an upload happens. But, if it's the former, then it'd definitely be useful for the comment to get updated immediately with ** OUTDATED ** or similar at the top of the comment.

Git Pushs/PR webhook events. It might not be as obvious as it cound me, but the wording in the time between the push and the upload says something to the effect of "We do not have coverage for the latest commit <SHA> on head. Please consider uploading"

drazisil-codecov avatar Dec 07 '23 13:12 drazisil-codecov

Git Pushs/PR webhook events. It might not be as obvious as it cound me, but the wording in the time between the push and the upload says something to the effect of "We do not have coverage for the latest commit <SHA> on head. Please consider uploading"

I don't see that at all (though it'd probably still be better to have something more noticeable either way): image

The comment appears to be edited after every upload, but everything it displays is still referring to the previous commit (as there aren't enough uploads for the current commit yet). I didn't see an edit happen before the first upload.

Dreamsorcerer avatar Dec 07 '23 16:12 Dreamsorcerer

Hi @Dreamsorcerer ,

Do you have the Codecov GitHub app installed? https://github.com/apps/codecov

It's possible we arn't getting the web hooks and are only updating on coverage uploads.

drazisil-codecov avatar Dec 07 '23 17:12 drazisil-codecov