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

Ensure API returns file list consistent with PR comment #143

Closed
2 tasks
Tracked by #142
trent-codecov opened this issue Aug 1, 2023 · 5 comments
Closed
2 tasks
Tracked by #142

Ensure API returns file list consistent with PR comment #143

trent-codecov opened this issue Aug 1, 2023 · 5 comments
Assignees
Labels
P0: must do priority 10

Comments

@trent-codecov
Copy link
Contributor

trent-codecov commented Aug 1, 2023

The PR comment is displaying files with 0% change. We want the UI (via the API) to display these files as well.

Tasks

@scott-codecov
Copy link

@aj-codecov Do we have an example of this not working somewhere in the wild? I just tested this and I see files with 0% change in both the PR comment as well as the dashboard (main/bar.py is a file w/ diff changes but no coverage changes).

PR comment:

Screenshot 2023-08-14 at 9 48 40 AM

Dashboard:

Screenshot 2023-08-14 at 9 49 02 AM

@scott-codecov
Copy link

Oh maybe this is an example? codecov/worker#61

The PR comment is showing test files b/c they're part of the diff. Is that the behavior we'd like to replicate in the dashboard? That honestly seems a little weird to me. I'd maybe suggest removing the test files from the PR comment instead. WDYT?

@aj-codecov
Copy link

@scott-codecov Yes, that's an example, basically those are files that are changed that can impact coverage within a PR, almost more of an informational thing than anything. @codecovdesign curious if you have thoughts here - I see the potential value in including this in the UI, but we'd need to probably put them in their own category, or label them somehow to call out that they're test files and thus different from everything else in the UI.

@scott-codecov
Copy link

scott-codecov commented Sep 6, 2023

Thanks @aj-codecov - I did a little more exploration here and better understand what's going on now. In worker we're actually including test coverage of the test files (which I think is where the "mistake" is). I think it makes sense to add something like this to our codecov.yml:

ignore:
  - "**tests**/test_*.py"

Regardless, there is still a discrepancy between the PR comment and the UI in that the PR comment will show files that are part of the diff AND have some coverage info even if there is no coverage change in the diff for that file. Question is - do we want to show those files in the UI as well?

Am I understanding that correctly?

@aj-codecov
Copy link

Scott and I chatted through this further in Slack, but the end result of that convo was that we want to include all files changed in a PR that could impact coverage in the "files changed" section of the pulls view.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0: must do priority 10
Projects
Status: Done
Development

No branches or pull requests

3 participants