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

Fix PR commit status failing due to Codecov #1021

Merged
merged 1 commit into from
May 5, 2024
Merged

Conversation

ehmicky
Copy link
Collaborator

@ehmicky ehmicky commented May 4, 2024

Codecov seems to take more than 1 hour to merge the test coverage from Linux + macOS + Windows. Due to this, the PR commit status initially fails.

I cannot find an easy to fix this, so this PR fixes it by reverting to the old behavior of only uploading the test coverage on Linux. This means Codecov will report ~99.90% test coverage even though it is actually 100%, but that's ok. :/

This also means the test coverage will decrease with this PR, making the commit status of this PR fail with Codecov.

@sindresorhus
Copy link
Owner

I cannot find an easy to fix this, so this PR fixes it by reverting to the old behavior of only uploading the test coverage on Linux. This means Codecov will report ~99.90% test coverage even though it is actually 100%, but that's ok. :/

Just use the ignore comment for that line: https://github.com/bcoe/c8?tab=readme-ov-file#ignoring-uncovered-lines-functions-and-blocks

That's what it's for. Cases that are tested, but missed in the coverage step.

@ehmicky ehmicky force-pushed the codecov-fix branch 2 times, most recently from 036b504 to 8d4698a Compare May 5, 2024 16:21
@ehmicky
Copy link
Collaborator Author

ehmicky commented May 5, 2024

(Accidentally hit the PR close button instead of updating this comment).

If I'm not mistaken, it's the Windows test coverage that is uploaded too late to Codecov, because Windows is just slower on GitHub actions. This means the difference in test coverage are the lines of code that are only run on Windows.

At the moment, we only have one line like this. However, once we merge node-cross-spawn (#578), it might be entire files. I have updated the PR as suggested though.

(I am putting this PR in draft, as I am trying to see if I can fix the Codecov configuration, in a separate PR)

@ehmicky ehmicky closed this May 5, 2024
@ehmicky ehmicky reopened this May 5, 2024
@ehmicky ehmicky marked this pull request as draft May 5, 2024 16:27
@ehmicky
Copy link
Collaborator Author

ehmicky commented May 5, 2024

It seems like there might be a way to fix this with the following Codecov configuration property: after_n_builds. If I understand this correctly, this means Codecov would only change the commit status once all of the 6 builds (3 OS * 2 Node versions) have completed.

This also means we would not need to skip any test coverage.

I have updated the PR with this. It seems to work. Shall we try this?

@ehmicky ehmicky marked this pull request as ready for review May 5, 2024 19:08
@sindresorhus sindresorhus merged commit e5df163 into main May 5, 2024
12 checks passed
@sindresorhus sindresorhus deleted the codecov-fix branch May 5, 2024 19:09
@sindresorhus
Copy link
Owner

Lets try 👍

@ehmicky
Copy link
Collaborator Author

ehmicky commented May 5, 2024

Seems to work in the 2 other open PRs. Let's see if it keeps working.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants