feat: add coverage for other services with tests #2405
Conversation
SkArchon
requested review from
a team,
Aenimus,
JivusAyrus,
Noroth,
StarpTech,
devsergiy,
endigma,
jensneuse,
thisisnithin and
wilsonrivera
as code owners
WalkthroughAdd coverage test targets and scripts across services, update CI workflows to run coverage and upload artifacts to Codecov (retention 14 days), extend post-merge Codecov workflow with additional workflow paths and a github-token input, and adjust repository Codecov thresholds and flags. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks✅ Passed checks (3 passed)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2405 +/- ##
===========================================
- Coverage 55.59% 42.75% -12.85%
===========================================
Files 213 1030 +817
Lines 23240 142648 +119408
Branches 0 8660 +8660
===========================================
+ Hits 12921 60984 +48063
- Misses 9072 79997 +70925
- Partials 1247 1667 +420 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Router image scan passed✅ No security vulnerabilities found in image: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/graphqlmetrics-ci.yaml (1)
61-75: Change test command tomake test-coverageto match upload expectations.Line 63 runs
make test, but the Codecov upload step (line 73) expectsgraphqlmetrics/coverage.outwhich is generated bymake test-coverage(as defined in graphqlmetrics/Makefile). The test step must be updated to generate the coverage output.Apply this diff to fix the test command:
- name: Test working-directory: ./graphqlmetrics - run: make test + run: make test-coverage - name: Build working-directory: ./graphqlmetrics run: make build - name: Upload integration results to Codecov uses: ./.github/actions/codecov-upload-pr with: artifact-name: graphqlmetrics-tests-coverage coverage-path: graphqlmetrics/coverage.out retention-days: 7 codecov-token: ${{ secrets.CODECOV_TOKEN }}.github/workflows/protographic.yaml (1)
6-7: Fix the workflow trigger path name mismatch.Line 7 references
".github/workflows/protographic-ci.yaml", but the current file is namedprotographic.yaml. This mismatch means the workflow will not trigger on changes to itself. Update line 7 to reference the correct filename.Apply this diff to fix the path:
- ".github/workflows/protographic-ci.yaml" + ".github/workflows/protographic.yaml"
🧹 Nitpick comments (1)
protographic/package.json (1)
35-35: Verify version compatibility between vitest and @vitest/coverage-v8.The
@vitest/coverage-v8dependency is pinned at version3.2.4, while thevitestdependency (line 37) uses the caret range^3.2.4, which allows updates up to<4.0.0. Ifvitestis upgraded to a later minor version (e.g., 3.3.x) without updating the coverage provider, there could be compatibility issues. Consider either pinning both to the same version or using a matching range for both.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
.github/workflows/composition-ci.yaml(2 hunks).github/workflows/controlplane-ci.yaml(2 hunks).github/workflows/graphqlmetrics-ci.yaml(1 hunks).github/workflows/protographic.yaml(1 hunks).github/workflows/router-ci.yaml(1 hunks).github/workflows/studio-ci.yaml(1 hunks)composition-go/Makefile(1 hunks)composition/package.json(2 hunks)controlplane/package.json(2 hunks)graphqlmetrics/Makefile(1 hunks)protographic/package.json(2 hunks)router/Makefile(2 hunks)studio/package.json(2 hunks)
🧰 Additional context used
🪛 Checkov (3.2.334)
.github/workflows/controlplane-ci.yaml
[medium] 86-87: Basic Auth Credentials
(CKV_SECRET_4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: build-router
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./events)
- GitHub Check: image_scan
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
🔇 Additional comments (13)
studio/package.json (2)
19-19: ✓ Test coverage script addition is well-formed.The new
test:coveragescript correctly extends the base test configuration with the--coverageflag while maintaining existing reporters. This aligns with the PR's objective to add coverage infrastructure.
137-137: Verify version consistency between vitest and @vitest/coverage-v8.The
@vitest/coverage-v8dependency is pinned to an exact version (3.2.4), whilevitestitself uses a caret range (^3.2.4 on line 147). This creates a potential version mismatch: ifvitestis upgraded to 3.2.5 or 3.3.0, the coverage adapter would remain at 3.2.4, risking incompatibility.Consider using a caret range for consistency:
- "@vitest/coverage-v8": "3.2.4", + "@vitest/coverage-v8": "^3.2.4",Alternatively, verify that this version pinning is intentional. Additionally, confirm that version 3.2.4 is valid and compatible with the vitest versions in the range ^3.2.4.
router/Makefile (2)
1-1: Coverage file variable introduction looks good.Extracting the coverage filename as a constant improves maintainability. The variable is properly used in the
test-coveragetarget.
6-6: Verify that theFORMATchange doesn't break local test workflows.The default test format for non-CI environments has been changed to
pkgname. This affects the output of both the existingtesttarget and the newtest-coveragetarget. Ensure that developers are aware of this formatting change and that it aligns with team expectations for local test output..github/workflows/studio-ci.yaml (1)
55-66: LGTM!Coverage integration looks correct. The multiple coverage paths (clover.xml and coverage-final.json) are standard outputs from Vitest with @vitest/coverage-v8, and artifact naming is descriptive.
composition-go/Makefile (1)
5-7: LGTM!The test-coverage target appropriately generates separate coverage profiles for normal and v8-tagged builds, enabling comprehensive coverage reporting for both build variants.
composition/package.json (1)
21-21: No action needed. @vitest/coverage-v8 v3.2.4 is compatible with vitest ^3.2.4—the coverage package explicitly requires vitest@3.2.4, which is satisfied by the ^3.2.4 constraint in the package.json.Likely an incorrect or invalid review comment.
controlplane/package.json (1)
26-26: Align vitest and @vitest/coverage-v8 version constraints.@vitest/coverage-v8@3.2.4 requires exactly vitest 3.2.4 as a peer dependency. Using
^3.2.4for vitest allows npm/pnpm to install vitest 3.3.0 or later, which would break compatibility with the pinned coverage plugin. Either pin vitest to3.2.4or upgrade @vitest/coverage-v8 to a version matching your intended vitest version..github/workflows/composition-ci.yaml (2)
47-51: Verify that the test command generates coverage artifacts.The test step at line 48 runs
pnpm run --filter composition test, but the subsequent Codecov upload (lines 53–61) expects coverage files atcomposition/coverage/clover.xmlandcomposition/coverage/coverage-final.json. Verify that either thetestscript has been updated to enable coverage collection, or this step should use atest-coveragescript instead.
68-74: Verify that Go test commands generate coverage artifacts.The Go test steps at lines 69–70 and 72–74 run standard
go testcommands, but the subsequent Codecov upload (lines 76–84) expects coverage files atcomposition-go/coverage_normal.outandcomposition-go/coverage_v8.out. Verify that these commands have been updated with-coverprofileflags to generate the expected coverage files, or that a separate coverage collection step exists (e.g., in a Makefile)..github/workflows/protographic.yaml (2)
38-39: Verify that the test:coverage script is properly defined and generates coverage artifacts.Line 39 updates the test command to use
test-coverage, which should invoke thetest:coveragescript added toprotographic/package.json. Ensure that this script (with the@vitest/coverage-v8provider) generates coverage files at the paths expected by the Codecov upload step:protographic/coverage/clover.xmlandprotographic/coverage/coverage-final.json.
44-52: Codecov upload step looks well-formed.The upload action, artifact naming, coverage paths, and Codecov token usage are appropriate. This step will execute after coverage artifacts are generated by the test-coverage step.
protographic/package.json (1)
23-24: Test:coverage script is correctly defined.The
test:coveragescript at line 24 uses the standard Vitest syntax to generate coverage reports with the v8 provider, which will produce theclover.xmlandcoverage-final.jsonfiles expected by the CI workflow.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
router/Makefile (1)
54-54: Complete the.PHONYdeclaration.While
test-coveragewas appropriately added to.PHONY, the declaration is still incomplete. Several other targets defined in the Makefile are missing:compose-demo-config,test-fresh,clean-testcache,build-race,build-custom, andgenerate-mocks. Adding all phony targets ensures Make always executes them regardless of filename collisions.Apply this diff to add the remaining phony targets:
-.PHONY: dev test test-coverage build lint bump-engine update-snapshot +.PHONY: dev test test-coverage build lint bump-engine update-snapshot \ + compose-demo-config test-fresh clean-testcache build-race build-custom \ + generate-mocks
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/codecov-post-merge.yaml(1 hunks).github/workflows/controlplane-ci.yaml(2 hunks).github/workflows/graphqlmetrics-ci.yaml(1 hunks).github/workflows/protographic.yaml(2 hunks)graphqlmetrics/Makefile(2 hunks)router/Makefile(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/protographic.yaml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-24T12:54:00.765Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2222
File: router-tests/websocket_test.go:2238-2302
Timestamp: 2025-09-24T12:54:00.765Z
Learning: The wundergraph/cosmo project uses Go 1.25 (Go 1.23+ minimum), so fmt.Appendf and other newer Go standard library functions are available and can be used without compatibility concerns.
Applied to files:
graphqlmetrics/Makefile
🪛 checkmake (0.2.2)
graphqlmetrics/Makefile
[warning] 26-26: Missing required phony target "all"
(minphony)
[warning] 26-26: Missing required phony target "clean"
(minphony)
router/Makefile
[warning] 54-54: Missing required phony target "all"
(minphony)
[warning] 54-54: Missing required phony target "clean"
(minphony)
🪛 Checkov (3.2.334)
.github/workflows/controlplane-ci.yaml
[medium] 86-87: Basic Auth Credentials
(CKV_SECRET_4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_test
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
router/Makefile (1)
1-1: Good implementation of test-coverage target.The
coverage_filevariable andtest-coveragetarget are properly configured. The use of-coverprofileand-coverpkgflags correctly captures coverage for the router package.Also applies to: 21-22
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/controlplane-ci.yaml (1)
115-115: Remove extraneous blank line for consistency.There's an extra blank line after
image_descriptionin thebuild_push_imagejob. While this is cosmetic, maintaining consistent formatting across the workflow improves readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/cli-ci.yaml(1 hunks).github/workflows/composition-ci.yaml(1 hunks).github/workflows/controlplane-ci.yaml(3 hunks).github/workflows/graphqlmetrics-ci.yaml(1 hunks).github/workflows/protographic.yaml(2 hunks).github/workflows/router-ci.yaml(3 hunks).github/workflows/studio-ci.yaml(1 hunks)CONTRIBUTING.md(1 hunks)codecov.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/cli-ci.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/protographic.yaml
- .github/workflows/graphqlmetrics-ci.yaml
- .github/workflows/studio-ci.yaml
🧰 Additional context used
🪛 actionlint (1.7.9)
.github/workflows/composition-ci.yaml
64-64: could not parse action metadata in "/home/jailuser/git/.github/actions/go-linter": line 5: unexpected key "type" for definition of input "working-directory"
(action)
🪛 Checkov (3.2.334)
.github/workflows/controlplane-ci.yaml
[medium] 86-87: Basic Auth Credentials
(CKV_SECRET_4)
🪛 LanguageTool
CONTRIBUTING.md
[grammar] ~173-~173: Use a hyphen to join words.
Context: ... merged PR is never present on main post merge, leading to no code coverage uploa...
(QB_NEW_EN_HYPHEN)
[style] ~174-~174: Consider a more concise word here.
Context: ...ch is used as a base for comparisons). In order to circumvent this we do the following ste...
(IN_ORDER_TO_PREMIUM)
[uncategorized] ~178-~178: The official name of this software platform is spelled with a capital “H”.
Context: ...using the GitHub rest API * We find the github artifact from this commit hash * We the...
(GITHUB)
[grammar] ~196-~196: Use a hyphen to join words.
Context: ...he cosmo repository root and it is comma separated, e.g.: for graphqlmetrics `,.g...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
CONTRIBUTING.md
185-185: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
193-193: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: build-router
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: image_scan
- GitHub Check: build_test
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_test
- GitHub Check: image_scan (nonroot)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build_test
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
codecov.yaml (1)
10-38: Configuration looks good, but verify the rationale for target reduction.The per-service flags with
carryforward: trueare correctly configured to prevent coverage from being reset when unrelated services are modified. However, the project default target reduction from 90 to 30 is a significant change. Ensure this aligns with your coverage baseline strategy—whether this is a deliberate step-wise increase path or a reflection of realistic current coverage across services.Please confirm the intent behind reducing the project patch target from 90 to 30.
.github/workflows/router-ci.yaml (1)
178-190: Codecov integration is properly configured.The coverage target and upload steps are correctly implemented with clear artifact naming and proper secret handling. Retention period aligns with other workflows.
.github/workflows/controlplane-ci.yaml (1)
84-96: Codecov integration correctly addresses previous feedback.The artifact name has been properly corrected to
controlplane-tests-coverageand the coverage upload is correctly configured. The vitest coverage format (clover.xml + coverage-final.json) is consistent with TypeScript testing practices..github/workflows/composition-ci.yaml (1)
48-84: Multi-language coverage collection is well-structured.The separation of TypeScript and Go coverage uploads with distinct artifact names and matched retention periods is a clean implementation. The Go test variants (normal and V8) are properly captured in separate profiles and combined in a single upload step.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CONTRIBUTING.md (1)
174-174: Fix verbose phrasing and GitHub brand capitalization.Line 174 uses unnecessarily verbose phrasing, and line 178 should capitalize "GitHub" per brand conventions.
-In order to circumvent this we do the following steps: +To work around this, we follow these steps: * Upload Codecov reports from PR runs to GitHub as artifacts for the PR's HEAD commit * Upon merge we find the PR and it's HEAD commit using the GitHub rest API -* We find the github artifact from this commit hash +* We find the GitHub artifact from this commit hashAlso applies to: 178-178
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CONTRIBUTING.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
CONTRIBUTING.md
[style] ~174-~174: Consider a more concise word here.
Context: ...ch is used as a base for comparisons). In order to circumvent this we do the following ste...
(IN_ORDER_TO_PREMIUM)
[uncategorized] ~178-~178: The official name of this software platform is spelled with a capital “H”.
Context: ...using the GitHub rest API * We find the github artifact from this commit hash * We the...
(GITHUB)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: build-router
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./events)
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
CONTRIBUTING.md (1)
181-196: Onboarding section is clear and well-documented.The steps for adding coverage to new projects are comprehensive and include proper YAML examples with language specifications. The guidance on carryforward flags and workflow-paths is helpful for contributors.
This PR adds
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.
Checklist