codecov / feedback

A place to discuss feedback about the pull request and web product experience.
35 stars 6 forks source link

Feedback about the new pull request comment #8

Open kylemann opened 2 years ago

kylemann commented 2 years ago

Thanks for dropping by! πŸ‘‹

We've been iterating and updating the layout, summary, and copy of the pull request comment.

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.

TechMaz commented 2 years ago

I think it's a great feature, but it can sometimes clog up a PR review screen, so it would be great if they were collapsible so that we can read the code around them more easily when desired.

codecovdesign commented 2 years ago

@TechMaz thank you for your feedback. We are actually experimenting with a new layout, see below. It prioritizes the high level summary and then collapses the other reporting data (impacted files, table, etc).

Would love to hear your feedback πŸ™

new_comment
openrefactory commented 2 years ago

@eliatcodecov Please watch this issue we have open this issue one week ago still don't get any respond yet.

https://github.com/httpie/httpie/pull/1434

daniellockyer commented 2 years ago

I love it! It's so much cleaner and less obtuse <3

unmade commented 2 years ago

I'm not sure how that's possible but pull request form shows me wrong coverage result - it says my pull request will decrease project coverage, although I'm confident my changes won't not affect coverage at all. I don't see that problem in the codecov web application.

I also compared coverage.xml files uploaded to codecov for corresponding commits and they're identical.

The pull request with wrong form: https://github.com/unmade/shelf-back/pull/97

Full report at codecov: https://app.codecov.io/gh/unmade/shelf-back/pull/97

Screenshot 2022-10-05 at 22 22 19

And here's the coverage files for corresponding commits:

nitram509 commented 2 years ago

Hi,

thanks for providing codecov tests to the Open Source community for free. After adding some tests, I found your report reported lower coverage than before, when no test was there at all. See https://github.com/argoproj/argo-rollouts/pull/2303#issuecomment-1272410769

I do not trust the codecov report, because why it's going down when I literally create the first test file in this package?

PS: I just measured the test coverage of the new file created, this is 87% (according to IntelliJ).

KnorpelSenf commented 1 year ago

This may sound like a nitpick but the coverage diff has a sign error in the GitHub comment that the bot is posting. It says "coverage decreases by -3%" but decreasing by a negative amount would mean increasing. Perhaps it should say "coverage decreases: -3%" to make it more accurate?

ssb22 commented 1 year ago

On this pull request I simply added some lines to the documentation (README.md and README.rst), but CodeCov warned that code coverage had been reduced by my change. It would be nice if obvious documentation files didn't count. Thanks.

mamil commented 1 year ago

Hi,

On this PR, I just copy an old case and then mod one function call, no tests were removed, and the test coverage decreased.

sayo96 commented 1 year ago

I'm unable to view the coverage report like before on the PR's. Codecov is such a big mess rn.

codecovdesign commented 1 year ago

"coverage decreases by -3%"...Perhaps it should say "coverage decreases: -3%" to make it more accurate?

Thank you, @KnorpelSenf that makes sense - will update text clarification in next iteration.

codecovdesign commented 1 year ago

unable to view the coverage report like before on the PR @sayo96 it's a new PR layout we are testing. is there something specific that you noticed and/or preferred to see that was in the the previous comment?

PaulRBerg commented 1 year ago

Is it possible to disable the automatic comments?

We'd like to keep using the Codecov GitHub app (so that our codecov.yml file gets synced with Codecov) but not receive the PR comments.

Update: it looks like comments can be disabled by adding comment: false in the Codecov config.

slifty commented 1 year ago

Is it possible to manually request that codecov re-run the report? I've found the most spotty aspect of codecov in our development workflow to be when we force push to a branch with new test coverage but codecov doesn't update the statistics.

It would be great to be able to basically say "@codecov rerun" or something along those lines in the PR.

ntindle commented 1 year ago

Hey! Over at https://github.com/Significant-Gravitas/Auto-GPT/, we are running into some severe problems with CodeCov. We tried reaching out to a rep and getting some of it worked out and a demo, but they were unavailable at our scheduled time. We liked CodeCov, but it currently needs fixing and denying all PRs. I don't particularly want to swap providers, but hundreds of people are asking us why CodeCov is broken, and we need help finding answers. I would love a reply as soon as you can

borislavnnikolov commented 1 year ago

What about the StateHasChanged implementation for the other components, like MudTabs and etc. ?

webbnh commented 1 year ago

The pull request comment seems good to me (I'm just starting with Codecov), but there seems to be a bootstrap problem which I describe here: when there is nothing to compare to (because the base commit doesn't have any code in it yet), it's treated as a error situation instead of as though all the changes are new code (which...they are 😁).

Once my PR is merged, we'll presumably never encounter the situation again, so perhaps it's not real important, but fixing it would improve the user's on-boarding experience, and it doesn't seem like it would be that hard to address.

codecovdesign commented 1 year ago

@webbnh thank you for your feedback!

Once my PR is merged, we'll presumably never encounter the situation again, so perhaps it's not real important, but fixing it would improve the user's on-boarding experience

πŸ’― that's right, the comment has a missing base report and looks like an error πŸ™‰ . We are actively looking to have the 1st comment more welcoming and clear as seen below. If you have any thoughts or revisions you'd make let me know!

1st_time_ux
iselo commented 1 year ago

Let's do the math!) Screen Shot 2023-07-15 at 3 59 46 PM

KnorpelSenf commented 1 year ago

Those are rounded numbers, so it could have gone down by 0.008 from 99.614 to 99.606 which would be rounded to 0.01 from 99.61 to 99.61. Perhaps we would want the diff to operate based on the rounded absolute values, but that could effectively hide a decrease in test coverage.

webbnh commented 1 year ago

Explicit rounding aside, floating point math is much harder than it seems. (Any time you see a decimal point in the middle of the number, assume that anything to the right of it is "an estimate"! 😁)

Gottox commented 1 year ago

Thanks for your great service!

I'm not sure if I'm doing smth wrong, but I get the following warning on my personal (!) project:

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

ref: https://github.com/Gottox/libsqsh/pull/88#issuecomment-1698853529

I also enabled the app for this account: image

Is that a false positive?

codecovdesign commented 1 year ago

@Gottox thank you for raising this πŸ™ . If the GitHub app is installed, this appears to be a bug and should not have been shown. I've created an bug issue and raised with the team.

yuekui commented 1 year ago

We've already setup comment without feedback: image

But why we're still getting this feedback section? image

codecovdesign commented 1 year ago

@yuekui thank you for surfacing this πŸ™. We investigated it and appears to have been a bug that is now fixed. Let me know if you're still seeing the issue!

karfau commented 1 year ago

image I took me a while to understand that this means the current run that uploads the report has not completed yet.

I would suggest to make this more obvious by changing the wording to something like

This the last uploaded report is for commit 1234567 but the latest commit for this PR is 4567890, this comment will be updated as soon as the report for that commit has been uploaded.

zimeg commented 10 months ago

πŸ‘‹ Hey there, big fan of PR comments and y'alls coverage checks in general! Really appreciate this place for feedback!

A recent change might have moved the `diff` into a details section with `files` being shown at the top level patch

Previously only the diff was shown with the same header and footer, without the specifics on files. I preferred that approach since it focused more on improving coverage in the patch instead of missing coverage[^1].

I found a section in the docs on changing this layout and was wondering if the comment example[^2] is accurate with the dropdown above? The codecov.yml used in the comment's project has no layout section at the moment.

Also wondering how adding condensed_ changes the comment layout? I'll probably try out a few different options soon but it's not super clear to me right now. Thanks for all of the great features! :raised_hands:

[^1]: πŸ˜‰ πŸ’š https://about.codecov.io/blog/the-case-against-100-code-coverage/ [^2]: Small note, there's an extra space before the hide_project_coverage field

rohan-at-sentry commented 10 months ago

πŸ‘‹ Hey there, big fan of PR comments and y'alls coverage checks in general! Really appreciate this place for feedback!

A recent change might have moved the diff into a details section with files being shown at the top level Previously only the diff was shown with the same header and footer, without the specifics on files. I preferred that approach since it focused more on improving coverage in the patch instead of missing coverage1.

I found a section in the docs on changing this layout and was wondering if the comment example2 is accurate with the dropdown above? The codecov.yml used in the comment's project has no layout section at the moment.

Also wondering how adding condensed_ changes the comment layout? I'll probably try out a few different options soon but it's not super clear to me right now. Thanks for all of the great features! πŸ™Œ

Footnotes

  1. πŸ˜‰ πŸ’š https://about.codecov.io/blog/the-case-against-100-code-coverage/ ↩
  2. Small note, there's an extra space before the hide_project_coverage field ↩

@zimeg thanks for the detailed feedback here πŸ‘πŸ½

You can bring back the older view by adding the following to your codecov.yml

layout: " header, diff, files, footer"

However, since you're open to it, I'd encourage experimenting with the options a little bit; there's a lot of good stuff in the new changes that I'm sure you'd benefit from.

Adding condensed_ accomplishes the following for each element

shlomi-noach commented 9 months ago

In this report, codecov tells me that there is 1 line missing coverage in my change:

image

However, clicking 1 missing, or any other link, only leads me to the full coverage breakdown of throttler.go, nothing to show me what specific 1 line was missing coverage in my change.

Link to OSS vitessio/vitess PR comment: https://github.com/vitessio/vitess/pull/14971#issuecomment-1895210210

EverWinter23 commented 8 months ago

It pollutes the entire PR, I cannot review the diff. Cause it pulls in unnecessary lines-of-code as well. This was not well thought out from UX point of view, I want to disable codecov just to get rid of the comments.

codecovdesign commented 8 months ago

@EverWinter23 thanks for the feedback, was hoping you could expand on it:

it pulls in unnecessary lines-of-code as well.

Do you have a screenshot of what your referring to? Any additional details can really help the team improve the format. Thank you πŸ™

donhcd commented 8 months ago

I'm getting a failing symbol ❌ in my PR checks

image

even though the comment on the same PR has a βœ… on it

image

ever since we introduced codecov to our project, probably 75+% of our PRs have had ❌s on them, probably due to this class of issue. In this specific case, my PR only affects github actions files -- why is codecov ❌ing me?

thomasrockhu-codecov commented 8 months ago

@donhcd can you provide a link to your PR or a commit SHA?

nhorton commented 8 months ago

Meta request: The way this solicitation of feedback is linked in the comment means that anything that syncs comments to places with previews (like Slack) results in a giant blob about this feedback request in those channels. If you made this go through a link shortener or such to break the preview, it would be really nice.

naik-aakash commented 7 months ago

The codecov report is bit inconsistent with on different runs for same test set.

https://github.com/JaGeo/LobsterPy/commits/main/

thomasrockhu-codecov commented 7 months ago

@naik-aakash can you be a little more specific about that? Which commits for example? Do you mean that the coverage results are different or that the PR comment format is different? Screenshots would be helpful as well.

naik-aakash commented 7 months ago

@naik-aakash can you be a little more specific about that? Which commits for example? Do you mean that the coverage results are different or that the PR comment format is different? Screenshots would be helpful as well.

Yes, sure, sorry. Coverage results are different with multiple runs. I suspect it had to do with layout_dicts.py file formatting now. Not entirely sure that was root cause for this or not.

Here is the PR where you can see the inconsistency.

https://github.com/JaGeo/LobsterPy/pull/245

HarelM commented 7 months ago

I'm not sure if this is only me reading this incorrectly, but I'm having a hard time truly understanding the difference between the PR comment and the report itself, they don't seem to be aligned: Relevant PR: https://github.com/maplibre/maplibre-gl-js/pull/3645 Screenshot of current comment: image Screenshot of the report in codecov: image

The number of missed lines is not the same. The patch percentage is not the same. I'm not sure which one of them should I trust... :-/

codecovdesign commented 7 months ago

@HarelM thank you for the detailed report πŸ™ I'm not sure what's happening here, but the results are clearly different. Our team is investigating it, created a bug report: https://github.com/codecov/engineering-team/issues/1321 Thanks again for sharing!

dabrahams commented 7 months ago

Is there a way to make codecov not send the report when all is well? I really only want to hear about failures. Thanks!

HarelM commented 7 months ago

It's also worth noting the following experience: The above library that I linked is sending multiple coverage files since some of the tests run on different workers in parallel. Codecov will create a coverage comment when the first file al finishes processing, so I get a mail notifying me with wrong coverage. In most cases this coverage comment will be updated although I've seen cases where it didn't. I would consider posting the coverage comment only when all the runners complete.

I know our use case might be complicated, but coverage is very important to our project, I've done a lot of tweaking to be able to measure coverage correctly.

codecovdesign commented 7 months ago

Is there a way to make codecov not send the report when all is well? I really only want to hear about failures.

@dabrahams there is this configuration that will only show if ANY changes occur. That doesn't quite acheive your ask though. cc'ing @thomasrockhu-codecov @drazisil-codecov are you familiar with another configuration to resolve this?

Created an issue to look at this more / adding ability to show only with negative reporting: https://github.com/codecov/engineering-team/issues/1350; please add any thoughts/comments you have.

codecovdesign commented 7 months ago

I get a mail notifying me with wrong coverage

@HarelM it may help to apply after_n_buid here, do you have this set? It will wait until that many reports are complete to update the comment. EDIT: adding issue to look at this further: https://github.com/codecov/engineering-team/issues/1352

HarelM commented 7 months ago

Ah, didn't know about this option, I'll try it out, thanks!

HarelM commented 7 months ago

@codecovdesign I don't see the specified flag in the github action parameters here: https://github.com/codecov/codecov-action Am I missing anything?

codecovdesign commented 7 months ago

@HarelM if updating flags you'll want to do this in the codecov.yaml https://docs.codecov.com/docs/flags#step-2-flag-management-in-yaml

HarelM commented 6 months ago

@codecovdesign is there a way to do it without introducing a whole new file to my repo in order to send a single flag to codecov? Is it possible to support the most common flag (or better yet all of them) in the github action configuration? I'm managing most of my CI configuration in this file anyway, it would be great to be able to simply add with: something or add some environment variables that the action will take into consideration.

HarelM commented 6 months ago

There's a bug that's preventing from getting accurate report due to the fact that it takes too many files (not the files I specifically requested): The following is the relevant bug: https://github.com/codecov/codecov-action/issues/1354 I think it can be considered a critical bug... Also I think that draft PR do not get coverage report due to some github API usage in codecov as far as I saw in the action run logs...

codecovdesign commented 6 months ago

@HarelM sorry missed this one; @drazisil-codecov and I were just looking at it and were unsure exactly. would you be up for a call? if so, shoot me an email at kyle.mann@sentry.io and will find a time. curious to learn more about your use case too.

HarelM commented 6 months ago

I've sent a mail to see when we can meet.