Pull Request Comment Report

codecovdesign commented 1 year ago

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.

bdaubry commented 1 year ago

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

New: Old:

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

eliatcodecov commented 1 year ago

@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"

bdaubry commented 1 year ago

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.

rohan-at-sentry commented 1 year ago

@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

bdaubry commented 1 year ago

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

aj-codecov commented 1 year ago

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

ludofischer commented 1 year ago

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.

bdaubry commented 1 year ago

@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:

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

aj-codecov commented 1 year ago

@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!

ludofischer commented 1 year ago

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

justinchuby commented 1 year ago

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.

aj-codecov commented 1 year ago

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"

jlbosse commented 1 year ago

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?

chadwhitacre commented 11 months ago

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

jerrodcodecov commented 11 months ago

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

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

chadwhitacre commented 11 months ago

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?

Dreamsorcerer commented 10 months ago

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

codecovdesign commented 10 months ago

@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! 🙏

Dreamsorcerer commented 10 months ago

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

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.

codecovdesign commented 10 months ago

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?

Dreamsorcerer commented 10 months ago

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.

HarelM commented 10 months ago

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.

Dreamsorcerer commented 10 months ago

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.

HarelM commented 10 months ago

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

Dreamsorcerer commented 10 months ago

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:

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 commented 10 months ago

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.

drazisil-codecov commented 10 months ago

👋 Also, the Edited word is a dropdown history of edits

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

drazisil-codecov commented 10 months ago

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"

Dreamsorcerer commented 10 months ago

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

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.

drazisil-codecov commented 10 months ago

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.

Dreamsorcerer commented 10 months ago

Yes, appears to be installed. Will keep an eye out on future PRs.

Dreamsorcerer commented 9 months ago

Another issue that could occur is some config goes wrong and coverage files aren't uploaded. For example, if you have Python and JS coverage, and then the JS coverage stops getting uploaded. I think at this point codecov just displays coverage of Python files and doesn't show any issues.

Again, the diff helps here by highlighting the large number of hits that have disappeared:

drazisil-codecov commented 9 months ago

Hi @Dreamsorcerer ,

If this is a matter of not all coverage being uploaded, https://docs.codecov.com/docs/carryforward-flags may be of help to you.

Dreamsorcerer commented 9 months ago

No, carryforward would hide the issue and we might never know that our coverage is broken. My point is that it can be easy to mess up some config or something and lose coverage on entire files without realising it (and end up with the state shown above). The problem is that the default view shows no coverage concerns, only expanding to see the diff do you see the glaring mistake.

I'm afraid the diff just seems to provide much better feedback than the default summary. I also have a project where the flags coverage is particularly important to see. Can we consider just having a config option for things that are displayed in the summary, above the collapsible section?

Firehed commented 6 months ago

It seems like there was another change recently removing even more detail, and I'm finding the general changes going from bad to worse. It's as if they're designed to push me into viewing the web UI on every PR (some engagement metric?) rather than giving me the information I need where I actually need it, as I used to get.

I now no longer get the "Additional details and impacted files" section, nor do I get the per-file information across the whole diff, only on files with partial coverage.

For me, the value of codecov isn't being able to see a UI of current coverage (though the historical trend is nice), as my test tools can provide that for me for free. Getting clear, inline feedback on my PR is where the value-add is. I want to log in to the web UI maybe once a quarter to get a summary of coverage and our overall progress, but absolutely do not want to have to click out of Github day to day. While I'm sure different users want different things out of the service, skimming the comments here suggests I'm not alone here.

What I want to see on every PR:

% coverage and % change of every file in the diff
% change to any other file in the project
Overall coverage and change

This is what I used to get, and what convinced me to get Codecov adopted at my day job. What I'm getting now is far too light on details. Of note:

Just telling me a file in the diff has 0% coverage gives me no context as to whether I forgot to add tests or the file has never had any (and it's not worth adding them)
It's non-obvious how my change is relative to the project as a whole
Not explicitly indicating my newly-added file that has 100% coverage makes me double-check if I actually committed the thing (and doesn't give me warm fuzzies, but whatever)

To be abundantly clear: every time I'm forced to visit the web UI during a PR to get the information I need, I'm mentally reassessing if there's a better coverage reporting tool I can use instead.

I'm happy that you've provided most of this functionality as an option inside codecov.yml as recommended above, and have been trying to bonk the config back into a place I'm happy with, but I'm still running into challenges:

The documentation is incomplete (several options mentioned in this thread are not documented), incorrect (the needed values are lowercased, the docs show Title Case), and unclear (e.g. files is apparently an alias for tree, not a different view on the data). These really need clear visual examples.
The data I want to see most is still forcibly stuck behind the details disclosure. It's a nice option for those that want it, but every step added to code review is another slowdown and loss of focus. Before I could skim without even taking my fingers off the keyboard.

(Update: the codecov.yml changes do not work reliably - most of the time I'm still getting only the new view)

I will say that despite my current negativity, I do rather prefer the new table format for the file list. The old xx% <yy%> (+z%) format was pretty hard to read, both visually and mentally. Having said that, I'd like to expand the current table view to have file-level columns in additional to the stats on the diff - the same data that was there before, but in the newer format. I don't want my team wasting time chasing 100% coverage on files that don't benefit from it, and that level of detail provides helpful context on "you forgot to add a test" vs "we're not bothering adding tests for this file right now"

codecovdesign commented 6 months ago

@Firehed thank you for the feedback/report 🙏 ! Could you help me troubleshoot the following:

I now no longer get the "Additional details and impacted files" (Related to above report: Just telling me a file in the diff has 0% coverage gives me no context It's non-obvious how my change is relative to the project as a whole)

For the referenced org/repo where you're not seeing "addtional details.."- Is that org/repo on the team plan + a private repo? If yes, this is intended as the team plan only shows patch reporting/summary. If not, could you link the PR if it's public (or clarify if it's on pro plan)? This would be a bug since oss/public/pro includes all report details/features.

the codecov.yml changes do not work reliably - most of the time I'm still getting only the new view

What configuration changes did you make?

Firehed commented 6 months ago

For the referenced org/repo where you're not seeing "addtional details.."- Is that org/repo on the team plan + a private repo? If yes, this is intended as the team plan only shows patch reporting/summary. If not, could you link the PR if it's public (or clarify if it's on pro plan)? This would be a bug since oss/public/pro includes all report details/features.

I believe we're on a team plan, and it's indeed a private repo.

Did this behavior recently change? I'm quite confident we'd been getting all of this info as recently as last week.

What configuration changes did you make?

I made the same edits as suggested at the top of this thread (comment.layout values), plus a number of variants based on the documentation page. When I was trial-and-erroring my way through the documentation to restore previous behavior sometimes I'd see impactful changes and other times it was as if the yaml file was completely ignored.

codecovdesign commented 6 months ago

Did this behavior recently change? I'm quite confident we'd been getting all of this info as recently as last week.

Yes, this was a result of a recent bug fix, that updated the Team plan to show as intended (patch only summary and changed files with missing coverage). Previously the OSS/Pro plan version was being displayed to the new Team plan (project coverage + additional details/features). This would also explain the configuration challenges you were having since they are not included in the pro plan.

I completely understand your frustration here and sorry this bug lead to confusion - we will be addressing these items. Some context: the team plan is a newly released plan geared toward smaller teams 11< that are looking for something more lightweight. Having said that, I'm bringing your feedback to the wider team for review, both generally but also specifically around your point about showing all files that have changed (not just files missing coverage). Thanks again and looking forward to our upcoming chat online 🙏

codecov / feedback

Pull Request Comment Report #54