-
Notifications
You must be signed in to change notification settings - Fork 48
Configure Codecov with coverage targets #337
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
Conversation
✅ Docs preview readyThe preview is ready to be viewed. View the preview File Changes 0 new, 1 changed, 0 removedBuild ID: 16251535d9ba8d25327bb1e2 URL: https://www.apollographql.com/docs/deploy-preview/16251535d9ba8d25327bb1e2 |
|
❌ Changeset file missing for PR All changes should include an associated changeset file. |
f6a9501 to
307b5ed
Compare
|
❌ Changeset file missing for PR All changes should include an associated changeset file. |
1 similar comment
|
❌ Changeset file missing for PR All changes should include an associated changeset file. |
ae28a27 to
8870ebc
Compare
|
❌ Changeset file missing for PR All changes should include an associated changeset file. |
|
❌ Changeset file missing for PR All changes should include an associated changeset file. |
|
❌ Changeset file missing for PR All changes should include an associated changeset file. |
| } | ||
| } | ||
|
|
||
| #[cfg(test)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added tests for untested code to verify that the Codecov configuration is working as expected.
https://app.codecov.io/github/apollographql/apollo-mcp-server/pull/337
| patch: | ||
| default: | ||
| # Require 80% coverage on all new/modified code | ||
| target: 80% |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I set the patch coverage target at 80% to begin with since that seems to be a reasonable standard in other Apollo projects. If anyone thinks this is too high or not realistic for our codebase, please let me know. Just to clarify, this requirement only applies to new or modified code, not untouched code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
80% seems fine to me. Is this possible to be overridden in the case where the SDK makes it near impossible to test, or we are rushing a hotfix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@swcollard We can use the #[coverage(off)] attribute from llvm-cov to exclude specific functions or modules from coverage reporting.
#![cfg_attr(coverage_nightly, feature(coverage_attribute))]
// function
#[cfg_attr(coverage_nightly, coverage(off))]
fn exclude_fn_from_coverage() {
// ...
}
// module
#[cfg_attr(coverage_nightly, coverage(off))]
mod exclude_mod_from_coverage {
// ...
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a Confluence page and added this for future reference.
| jobs: | ||
| verify-changeset: | ||
| if: ${{ !contains(github.event.pull_request.labels.*.name, 'skip-changeset') && !startsWith(github.head_ref, 'sync/') && !startsWith(github.head_ref, 'conflict/') }} | ||
| if: ${{ !contains(github.event.pull_request.labels.*.name, 'skip-changeset') && !startsWith(github.head_ref, 'sync/') && !startsWith(github.head_ref, 'conflict/') && !github.event.pull_request.draft }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I often find it difficult to decide what to write when drafting a PR. This will help reduce the comment noises on draft PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, should it have flagged this PR once it was moved out of draft status?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this! I'll update the workflow to include the ready_for_review event so it will trigger automatically when a PR is converted from draft to ready for review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
❌ Changeset file missing for PR All changes should include an associated changeset file. |
fcca1ec to
8283676
Compare


This PR adds
codecov.ymlto set up Codecov with specific coverage targets and quality standards. It helps define clear expectations for code quality. It also includes some documentation about code coverage inCONTRIBUTING.mdand adds the Codecov badge toREADME.md.