Skip to content

Conversation

@michellethomas
Copy link
Contributor

@michellethomas michellethomas commented May 2, 2019

CATEGORY

Choose one

  • Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

SUMMARY

In SIP-13 we discussed having automated checks for code coverage (section [P.5.3]). This PR creates a codecov yaml for us to set the targets and thresholds we want, so if we add a setting in the repo to alert if these rules aren't met, we can change them if we feel they are too strict.

Questions:

Should we have folders we ignore? I initially put in the visualizations folder to ignore because it's been difficult to add unit tests to cover this code, is that what we want? Are there other folders to ignore?

TEST PLAN

Used codecov's validator curl --data-binary @.codecov.yml https://codecov.io/validate
Tested on a PR on in my fork

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

REVIEWERS

@mistercrunch @kristw @john-bodley @xtinec @DiggidyDave

@codecov-io
Copy link

codecov-io commented May 2, 2019

Codecov Report

Merging #7433 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #7433   +/-   ##
=======================================
  Coverage   65.56%   65.56%           
=======================================
  Files         435      435           
  Lines       21745    21745           
  Branches     2393     2393           
=======================================
  Hits        14258    14258           
  Misses       7365     7365           
  Partials      122      122

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45b41aa...5bd05b5. Read the comment docs.

@michellethomas michellethomas changed the title [WIP] Adding codecov targets and thresholds Adding codecov targets and thresholds May 24, 2019
@michellethomas
Copy link
Contributor Author

michellethomas commented May 24, 2019

After discussing this, it seemed like there was general support for setting a patch target. But in the interest of getting this in sooner rather than later, I set a 1% threshold instead which I believe means it's going to look at the coverage of our repo and flag if the diff coverage is lower by > 1% from that number. And we can increase it over time if we want.

What do you all think about ignoring the visualization folder, do we want to do that?

@DiggidyDave
Copy link
Contributor

DiggidyDave commented May 24, 2019

I was actually going to reach out to you today about this, I'm in favor of it 100%. I personally would actually be pretty draconian about it--at least as a starting point--meaning:

  • don't omit any folders
  • don't allow any reduction, even <1%

But getting any form of this in ASAP is a huge step forward.

.codecov.yml Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

as noted in other comment, can we hold a hard line on this and disallow any reduction in coverage?

Also, purely out of curiosity: is there a way to bypass the check (with committer approval) in an emergency situation?

Copy link
Member

Choose a reason for hiding this comment

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

Nope I think you need admin to override the blockers, which none of the committers get. You'd have to open an apache INFRA ticket :(

Copy link
Contributor

Choose a reason for hiding this comment

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

It just occurred to me that this file provides a perfectly suitable (and auditable!) mechanism for emergency exemptions... the committer could just add one or more ignore paths with a comment in the file documenting the need, so we would have a written record of anything that accidentally gets left in there

@michellethomas
Copy link
Contributor Author

michellethomas commented May 24, 2019

The reason it may be useful to ignore the visualizations folder is that it's pretty difficult to add unit tests on that code, but I'm fine to remove the ignore if it doesn't seem useful.

@michellethomas
Copy link
Contributor Author

Set the threshold to 0% based on @DiggidyDave's suggestion and removed ignore directory. Anyone have any final comments? Once we get this in I'll need to get the repo to add this as a commit check. Until then it won't change anything. Seems good to just get it in and then we can adjust as necessary.

@DiggidyDave
Copy link
Contributor

SWEEEET

@michellethomas michellethomas merged commit 6d1f6e9 into apache:master Jun 3, 2019
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.34.0 First shipped in 0.34.0 labels Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/S 🚢 0.34.0 First shipped in 0.34.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants