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

Moving to CodeCov #1826

Merged
merged 9 commits into from Dec 6, 2018

Conversation

2 participants
@smashwilson
Copy link
Member

smashwilson commented Dec 5, 2018

Please be sure to read the contributor's guide to the GitHub package before submitting any pull requests.

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • Suggestion: You can use checklists to keep track of progress for the sections on metrics, tests, documentation, and user research.

Description of the Change

Coveralls has started lagging and failing to process uploaded coverage reports, and the team seems to be unresponsive: the company twitter account hasn't posted in a year, nobody has been responding on their public issue tracker, and so forth. I'm exploring CodeCov as a possible alternative.

Alternate Designs

I took a little time to look around at other code coverage apps on GitHub marketplace. I've seen a few other GitHub teams using CodeCov and it looks actively maintained, so it seemed like a good candidate to start with.

Benefits

We'll be able to get rid of our Travis CI build. Consolidating CI providers is always nice.

Also, It looks like CodeCov will merge any coverage reports received before the commit status changes to green. That'll be helpful for us because it means we'll get a unified coverage report across our entire OS and channel matrix - our Coveralls report only includes a Linux build on dev, so anything we gate by process.platform gets missed.

Possible Drawbacks

I haven't had a chance to look through the way that CodeCov lets you navigate through a coverage result report. The "killer feature" that Coveralls gave us was the ability to see quickly see both new and existing uncovered lines by comparing coverage to master. If that isn't as easy, we'll have to evaluate if the reliability gains still make it worth converting over.

Applicable Issues

n/a

Metrics

n/a

Tests

You'll see my testing live in real time on this PR by all of my "wtf" and "just work already" commits that fiddle with environment variables and configuration.

  • Update required status checks on this repo

Documentation

  • Update the coverage badge in the README.

Release Notes

n/a

User Experience Research (Optional)

n/a

smashwilson added some commits Dec 5, 2018

smashwilson added some commits Dec 5, 2018

@smashwilson smashwilson requested a review from atom/github-package Dec 5, 2018

@kuychaco
Copy link
Member

kuychaco left a comment

This looks good! Thanks @smashwilson for tackling this

I left one inline comment, and have a couple of questions for clarification.

moving_to_codecov_by_smashwilson_ _pull_request__1826_ _atom_github

☝️ I notice we have both project and patch status. Based on https://docs.codecov.io/docs/commit-status#section-changes-status it seems that we also want change status, yeah? To catch regressions in test coverage.

Additionally, there is a red-X for codecov/project blocking merge. Presumably because there is a drop of 0.04% in coverage. Maybe we want to set a threshold for posting success status? https://docs.codecov.io/docs/commit-status#section-threshold

Also, It looks like CodeCov will merge any coverage reports received before the commit status changes to green. That'll be helpful for us because it means we'll get a unified coverage report across our entire OS and channel matrix...

Not sure what you mean by that first sentence. Could you please clarify?

Show resolved Hide resolved azure-pipelines.yml Outdated

kuychaco and others added some commits Dec 5, 2018

@smashwilson

This comment has been minimized.

Copy link
Member

smashwilson commented Dec 6, 2018

Alrighty, I've configured it to allow up to 2% slack in coverage percentage, which should give us enough breathing room to account for our nondeterministic coverage flapping. I've also enabled the /changes status which should catch unexpected, unrelated decreases in coverage from PRs.

☝️ I notice we have both project and patch status. Based on https://docs.codecov.io/docs/commit-status#section-changes-status it seems that we also want change status, yeah? To catch regressions in test coverage.

Yup! And it's off by default, although I'm not entirely sure why:

coverage:
  status:
    project: yes
    patch: yes
    changes: no

I've turned it on and given it the same delta tolerance we have for the other status checks.

Additionally, there is a red-X for codecov/project blocking merge. Presumably because there is a drop of 0.04% in coverage. Maybe we want to set a threshold for posting success status?

Also done 👍

I'd love to get us to a place where our coverage is predictable enough we wouldn't have to have that slack at all. I don't want to hold up PR merges while we're getting to that point, though.

Not sure what you mean by that first sentence. Could you please clarify?

Our Azure DevOps build has a matrix of nine separate builds that run the test suite: one for each of the three Atom release channels (stable, beta, and dev) on each of the three operating systems (macOS, Windows, and Linux). All nine of these produce distinct .lcov coverage reports which get posted to the CodeCov API, all based on the same commit. They won't all be exactly the same! If we have any logic like this:

if (process.platform !== 'win32') {
  // ...
}

The "if" branch will be covered only on the six Linux and macOS builds, for example. Similarly, if there's anywhere that we test for some functionality in the Atom API that hasn't made it the whole way through stable yet, those "if" blocks will only be covered on dev or dev and beta builds.

Coveralls had a system we could have used to unify these, but because I could only get Coveralls working from Travis, it didn't matter - we only had a single Linux build that produced a single .lcov report.

CodeCov, on the other hand, does this automatically. Basically it watches the other statuses reported on the same commit and merges in any reports it's given for the same commit sha before CI has reported a final green or red status.

It's a nice bit of bonus functionality that we get without extra work, and as a result we'll be able to see coverage across platforms and across channels, which will be nice.

Stability Sprint : 20 November 2018 - 8 January 2019 : v0.24.0 automation moved this from In Progress 🔧 to QA Review 🔬 Dec 6, 2018

@kuychaco
Copy link
Member

kuychaco left a comment

:shipit:

@kuychaco kuychaco merged commit 55a6d63 into master Dec 6, 2018

3 checks passed

codecov/changes No unexpected coverage changes found.
Details
codecov/patch Coverage not affected when comparing 1bfec72...203a382
Details
codecov/project 89.53% (+0.07%) compared to 1bfec72
Details

Stability Sprint : 20 November 2018 - 8 January 2019 : v0.24.0 automation moved this from QA Review 🔬 to Merged ☑️ Dec 6, 2018

@kuychaco kuychaco deleted the aw/codecov branch Dec 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment