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

Pull Request Comment Report #54

Open
codecovdesign opened this issue Aug 15, 2023 · 38 comments
Open

Pull Request Comment Report #54

codecovdesign opened this issue Aug 15, 2023 · 38 comments
Labels
Feedback For gathering customer feedback

Comments

@codecovdesign
Copy link

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

bdaubry commented Aug 31, 2023

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?

@eliatcodecov
Copy link
Contributor

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

bdaubry commented Sep 5, 2023

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

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

bdaubry commented Sep 21, 2023

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

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

@ludofischer
Copy link

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

bdaubry commented Sep 28, 2023

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

@aj-codecov
Copy link
Member

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

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

@codecovdesign
Copy link
Author

@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

@Dreamsorcerer
Copy link

Dreamsorcerer commented Nov 27, 2023

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.

@codecovdesign
Copy link
Author

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: codecov/engineering-team#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
Copy link

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

HarelM commented Dec 6, 2023

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:
maplibre/maplibre-gl-js#3431 (comment)

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

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

HarelM commented Dec 7, 2023

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

Dreamsorcerer commented Dec 7, 2023

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

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

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

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

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.

@drazisil-codecov
Copy link

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

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

@Dreamsorcerer
Copy link

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

@drazisil-codecov
Copy link

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

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

Firehed commented Apr 2, 2024

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

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

Firehed commented Apr 2, 2024

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

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 🙏

@thomasrockhu-codecov thomasrockhu-codecov added the Feedback For gathering customer feedback label May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feedback For gathering customer feedback
Projects
Status: Waiting for: Product Owner
Development

No branches or pull requests