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
Comments
@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
|
Exactly what I needed, thank you!
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 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 Can you post a screenshot of your current PR comment? I think we can get you what you need |
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. |
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! |
@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! |
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. |
@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.
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! 🙏 |
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): If indirect changes are included in that summary, then it's probably fine. |
Thank you @Dreamsorcerer! This is really helpful 🙏
That is true for now. I created an issue to capture the problem and review further with the team: codecov/engineering-team#865
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. |
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: 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. |
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.
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. |
I'm sorry to be blunt here, but that doesn't make sense from a user experience point of view...
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... |
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 |
👋 Also, the Edited word is a dropdown history of edits I'll leave all the design feedback for @codecovdesign to handle :D |
Although ...
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 |
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. |
Yes, appears to be installed. Will keep an eye out on future PRs. |
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. |
No, carryforward would hide the issue and we might never know that our coverage is broken. 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? |
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:
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:
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:
(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 |
@Firehed thank you for the feedback/report 🙏 ! Could you help me troubleshoot the following:
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.
What configuration changes did you make? |
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.
I made the same edits as suggested at the top of this thread ( |
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 🙏 |
Thanks for dropping by! 👋
We'd love your feedback about the pull request comment, focused on the comment report details, layout, and information presented.
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.
The text was updated successfully, but these errors were encountered: