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

Total regression in codecov coverage report looks suspicious #35759

Closed
puzpuzpuz opened this issue Oct 22, 2020 · 14 comments
Closed

Total regression in codecov coverage report looks suspicious #35759

puzpuzpuz opened this issue Oct 22, 2020 · 14 comments
Labels
test Issues and PRs related to the tests.

Comments

@puzpuzpuz
Copy link
Member

Got an automated coverage report on #35753. The reported coverage regression is estimated as -8.50% which is very suspicious, considering that the changes made by the PR are relatively small and tests are included. Impacted files list also includes mostly unrelated files. Is it possible that there is something wrong with codecov configuration or the service itself?

See #35753 (comment) and https://codecov.io/gh/nodejs/node/pull/35753 for more details

@puzpuzpuz puzpuzpuz added the test Issues and PRs related to the tests. label Oct 22, 2020
@richardlau
Copy link
Member

#35670 (comment) is probably relevant.

cc @bcoe

@bcoe
Copy link
Contributor

bcoe commented Oct 23, 2020

@puzpuzpuz @richardlau I'm not 100% sure the reason, but codecov.io seems to get a bit confused about the order that commits land to our main branch:

Screen Shot 2020-10-22 at 6 59 38 PM

☝️ it's expected that coverage will be about 87% now, but it doesn't look like codecov.io has caught up yet.

I believe the notifications will stabilize once it catches up.

@puzpuzpuz
Copy link
Member Author

@bcoe thanks for the clarification. It makes sense. I'm going to hide the codecov message and close this issue. Feel free to reopen it if you think it's necessary.

@bcoe
Copy link
Contributor

bcoe commented Oct 24, 2020

see also: #35779

@bcoe
Copy link
Contributor

bcoe commented Oct 24, 2020

@thomasrockhu codecov's comments on PRs seem to behave somewhat strangely on the Node.js project:

The HEAD of our default branch has been at 87% coverage for several commits now, but the coverage graph oscillates:

image

Similarly, PRs are getting reports of seemingly unrelated drops in coverage.


I wonder if the problem might be dates on commits, if we look at the last N commits to the Node.js project:

Date: Sat Oct 17 19:38:35 2020 +0200
Date: Fri Oct 16 11:50:00 2020 -0600
Date: Sat Aug 29 16:20:44 2020 +0900
Date: Fri Oct 23 23:53:41 2020 +0200
Date: Sat Oct 17 04:49:05 2020 -0700

Note that the ordering of commit dates does not match the order in which they're landed.

I'm not 100% sure why our commit history looks like this (CC: @Trott, @nodejs/build).

@Trott
Copy link
Member

Trott commented Oct 25, 2020

Note that the ordering of commit dates does not match the order in which they're landed.

That's the date the commit was authored, not merged. That sort of thing will happen on any large codebase with lots of people working on it. For example, here are the last 10 timestamps as of this writing for https://github.com/microsoft/vscode:

Date:   Sat Oct 24 06:21:24 2020 +0000
Date:   Fri Oct 23 18:37:39 2020 -0700
Date:   Fri Oct 23 14:38:26 2020 -0700
Date:   Fri Oct 23 12:10:43 2020 -0700
Date:   Fri Oct 23 16:13:16 2020 -0700
Date:   Fri Oct 23 16:09:24 2020 -0700
Date:   Fri Oct 23 14:47:55 2020 -0700
Date:   Fri Oct 23 14:32:42 2020 -0700
Date:   Sat Oct 24 00:08:57 2020 +0300
Date:   Fri Oct 23 13:38:15 2020 -0700

If you run git log --format=fuller, you will get separate AuthorDate and CommitDate fields. I'm not sure the commit dates will necessarily be in order either, though, if there are force pushes or perhaps other things. But I suspect they will more likely be in the order you expect. This is, I think, what the GitHub interface uses.

@bcoe
Copy link
Contributor

bcoe commented Oct 25, 2020

If you run git log --format=fuller, you will get separate AuthorDate and CommitDate fields

@Trott thanks for clarifying, and providing the one liner, I figured it was something along these lines.

@Trott @thomasrockhu, looking at the list of recent commits on the default branch. It jumps out at me that codecov.io seems to be sorting by AuthorDate, whereas for providing an accurate comparison report on a PR, we would want to compare against the greatest CommitDate.

@Qard
Copy link
Member

Qard commented Oct 25, 2020

In #35779, the c8 step spat out a whole bunch of errors. Might be related: https://github.com/nodejs/node/runs/1299567071#step:8:1

@bcoe
Copy link
Contributor

bcoe commented Oct 25, 2020

In #35779, the c8 step spat out a whole bunch of errors. Might be related

@Qard these are warnings are noisy, but I believe are unrelated (they relate to the file path not being properly calculated for files in the deps/ folder, this information is then dropped in the final reports.).

Your coverage output is what I expect ~87% is the combined coverage for our C++ and JS dependencies. What seems to be broken is the comparison between this and the default branch, which consistently indicates drops which aren't accurate.

@thomasrockhu
Copy link

thomasrockhu commented Oct 26, 2020

@bcoe, I'm off this week, but I'll do a deep dive on Monday. My suspicion is that there might be something in the differing number of uploads between the ~87% covered commits (2 uploads) and the ~96% covered commits (1 upload).

I would just double-check this article in case anything there is useful.

@thomasrockhu
Copy link

@bcoe, looking at the graph here it looks like things have stabilized on the master branch. I wanted to address some of the things that have come up to make sure there isn't anything outstanding.

It jumps out at me that codecov.io seems to be sorting by AuthorDate, whereas for providing an accurate comparison report on a PR, we would want to compare against the greatest CommitDate.

I have let the product team know about this feedback, thanks so much. I'll get back to you if we have any other questions.

What seems to be broken is the comparison between this and the default branch, which consistently indicates drops which aren't accurate.

As I mentioned, all of the commits that were still at ~96% had only one build, which looks like a different coverage format than the commits with 2 builds. I think this is a result of PRs not having this commit and the comment you made. Does this make sense?

Folks are reporting that codecov.io is prompting them to authenticate to see the public reports:

Is this still an issue for your developers? We have since made fixes here.

@bcoe
Copy link
Contributor

bcoe commented Nov 4, 2020

As I mentioned, all of the commits that were still at ~96% had only one build, which looks like a different coverage format than the commits with 2 builds. I think this is a result of PRs not having this commit and the comment you made. Does this make sense?

The drop from 96% to 88% was expected; we added C++ coverage, which has a lower percentage than our JavaScript suite, so we saw a drop in coverage when it was merged.

The behavior that has been strange, is that it doesn't seem like opened PRs are comparing against the HEAD of our default branch, rather they are comparing against whichever commit has the highest authoring date.

Is this still an issue for your developers? We have since made fixes here.

I haven't seen it myself recently, but we're not currently reporting results on a per-PR basis, so there's less incentive for folks to click through to the report.


In general, it seems like things have been working well -- I would like to eventually get us reporting coverage information to PRs again, but our nightly reports have seemed healthy.

@thomasrockhu
Copy link

The behavior that has been strange, is that it doesn't seem like opened PRs are comparing against the HEAD of our default branch, rather they are comparing against whichever commit has the highest authoring date.

Is there an instance here that has been strange? Would love to dig into this and see if there's a bug or a misunderstanding.


At the end of the day, if you're happy with the way things are working now, that's what's important. Let me know if there's anything else I can help with here.

@bcoe
Copy link
Contributor

bcoe commented Jul 25, 2024

When this issue was opened, Codecov was looking at author date on commits. I believe they are now looking at committer date so I believe this issue is likely solved.

Config is updated here: #54019

@Trott Trott closed this as completed Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

No branches or pull requests

6 participants