-
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
Changes from all commits
8a138af
8870ebc
e771d36
8431bf6
47433b1
1c4dbdc
8283676
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| ### Configure Codecov with coverage targets - @DaleSeo PR #337 | ||
|
|
||
| This PR adds `codecov.yml` to 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 in `CONTRIBUTING.md` and adds the Codecov badge to `README.md`. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| coverage: | ||
| status: | ||
| project: | ||
| default: | ||
| # Should increase overall coverage on each PR | ||
| target: auto | ||
| patch: | ||
| default: | ||
| # Require 80% coverage on all new/modified code | ||
| target: 80% | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @swcollard We can use the #![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 {
// ...
}
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I made a Confluence page and added this for future reference. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| use crate::uplink::schema::SchemaState; | ||
| use super::SchemaState; | ||
| use std::fmt::Debug; | ||
| use std::fmt::Formatter; | ||
| use std::fmt::Result; | ||
|
|
||
| /// Schema events | ||
| pub enum Event { | ||
|
|
@@ -12,7 +13,7 @@ pub enum Event { | |
| } | ||
|
|
||
| impl Debug for Event { | ||
| fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { | ||
| fn fmt(&self, f: &mut Formatter) -> Result { | ||
| match self { | ||
| Event::UpdateSchema(_) => { | ||
| write!(f, "UpdateSchema(<redacted>)") | ||
|
|
@@ -23,3 +24,28 @@ impl Debug for Event { | |
| } | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn test_debug_event_no_more_schema() { | ||
| let event = Event::NoMoreSchema; | ||
| let output = format!("{:?}", event); | ||
| assert_eq!(output, "NoMoreSchema"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_debug_redacts_update_schema() { | ||
| let event = Event::UpdateSchema(SchemaState { | ||
| sdl: "type Query { hello: String }".to_string(), | ||
| launch_id: Some("test-launch-123".to_string()), | ||
| }); | ||
|
|
||
| let output = format!("{:?}", event); | ||
| assert_eq!(output, "UpdateSchema(<redacted>)"); | ||
| assert!(!output.contains("type Query")); | ||
| assert!(!output.contains("test-launch-123")); | ||
| } | ||
| } | ||


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_reviewevent so it will trigger automatically when a PR is converted from draft to ready for review.Uh oh!
There was an error while loading. Please reload this page.
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 tested the updated workflow on this PR.
When I removed the changeset and marked the PR as draft, the workflow correctly skipped verifying the changeset.
After marking the PR as ready for review, the verification ran and the expected comment was added.