Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Conversation

@smashwilson
Copy link
Contributor

@smashwilson smashwilson commented Dec 6, 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

Disable the "changes" status reported by CodeCov.

Alternate Designs

I'd originally tried to build us some slack by configuring it with a threshold key, but that doesn't appear to work. In retrospect, that makes sense - the changes status is reported as a total number of files, not by coverage percentage like the others are - and the commit status docs kind of support this.

Benefit

It'll unblock pull requests like #1822 that currently have a red "changes" check that we can't do anything about from merging.

It'll also prevent our CI-watching chatops from spamming the #atom channel on every merge to master.

Possible Drawbacks

There are some situations where we'll be able to incidentally decrease coverage and still get passing statuses from CodeCov. Deleting tests is one example.

Applicable Issues

n/a

Metrics

n/a

Tests

I'm going to see if this pull request doesn't have the "changes" status reported from CodeCov. 🙃

Documentation

n/a

Release Notes

n/a

User Experience Research (Optional)

n/a

@smashwilson smashwilson added the ci label Dec 6, 2018
@smashwilson smashwilson requested a review from a team December 6, 2018 13:45
@smashwilson
Copy link
Contributor Author

I've also removed codecov/changes from the required statuses in our protected branches config.

Reviewers: how do you feel about this? My intention would be re-enable this if and when we get to a point where our test coverage is stable. I think there are only a few lines that are causing coverage flapping so I don't think that's too far off.

@kuychaco
Copy link
Contributor

kuychaco commented Dec 6, 2018

I think there are only a few lines that are causing coverage flapping so I don't think that's too far off

Any idea what lines? Maybe it's worth trying to fix them upfront?

Also, perhaps it's worth reaching out to Codecov support with this question/issue? They seem to be very welcoming of this... they have an "Ask Codecov staff to review reports" section here

@smashwilson
Copy link
Contributor Author

Any idea what lines? Maybe it's worth trying to fix them upfront?

Back on Coveralls I consistently saw a few lines in Repository and I think RootController or GithubPackage that would flap coverage. But I don't have a really good way to see which lines they were right now.

I think the changes status is supposed to link to a view that shows them to us once we have a base commit with a coverage report? The page doesn't load for me, though, which probably is worth reaching out to support about.

Copy link
Contributor

@kuychaco kuychaco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 on disabling the Changes Status temporarily to unblock other PRs, reaching out to Codecov support, and re-enabling once we figure out what the deal is. Thanks @smashwilson!

@smashwilson smashwilson merged commit 869d817 into master Dec 6, 2018
@smashwilson smashwilson deleted the aw/codecov-changes-off branch December 6, 2018 19:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants