ci: break out integration tests from unit #2369
Conversation
WalkthroughThis update restructures test workflows and scripts to better separate unit and integration tests. It introduces new build constraints to distinguish integration tests, adds new Makefile targets for integration testing and coverage, and modifies CI to collect and combine coverage from both unit and integration tests before uploading to Codecov. The test for header sync service restart now uses a fresh datastore on restart. Changes
Sequence Diagram(s)sequenceDiagram
participant CI
participant UnitTest
participant IntegrationTest
participant ArtifactStore
participant CoverageCombiner
participant Codecov
CI->>UnitTest: Run unit tests
UnitTest->>ArtifactStore: Upload unit test coverage artifact
CI->>IntegrationTest: Run integration tests
IntegrationTest->>ArtifactStore: Upload integration test coverage artifact
CI->>CoverageCombiner: Combine coverage artifacts
CoverageCombiner->>ArtifactStore: Download unit & integration coverage
CoverageCombiner->>Codecov: Upload combined coverage
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (8)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
The latest Buf updates on your PR. Results from workflow CI and Release / buf-check (pull_request).
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…in permissions Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
| name: integration-test-coverage-report-${{ github.sha }} | ||
| path: ./integration-coverage | ||
| - name: Upload combined coverage report | ||
| uses: codecov/codecov-action@v5.4.3 |
Check warning
Code scanning / CodeQL
Unpinned tag for a non-immutable Action in workflow Medium test
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2369 +/- ##
==========================================
+ Coverage 69.70% 69.76% +0.06%
==========================================
Files 64 64
Lines 6241 6241
==========================================
+ Hits 4350 4354 +4
+ Misses 1501 1497 -4
Partials 390 390
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/test.yml (1)
150-154: 🛠️ Refactor suggestionUnpinned 3rd-party Action – security hardening
codecov/codecov-action@v5.4.3is a version tag, not a commit SHA.
Pin to a full SHA per GHAS recommendation:- uses: codecov/codecov-action@v5.4.3 + uses: codecov/codecov-action@ae5e... # exact commit🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 150-150: Unpinned tag for a non-immutable Action in workflow
Unpinned 3rd party Action 'Tests / Code Coverage' step Uses Step uses 'codecov/codecov-action' with ref 'v5.4.3', not a pinned commit hash
🧹 Nitpick comments (7)
node/helpers_test.go (1)
1-1: Avoid file-widenolint– tame the warning closer to the sourceA blanket
//nolint:unusedsuppresses all unused-code warnings for the whole file and can hide genuine mistakes.
Consider either (a) removing the directive entirely, or (b) moving it to the specific helpers that are intentionally left unused.-//nolint:unused +// nolint:unused // keep only if absolutely sure the whole file should skip the checknode/helpers.go (1)
1-1: Duplicate “unused” suppression – consider trimmingYou now have a file-level
//nolint:unusedas well as one onsafeClose.
If onlysafeCloseneeds the exemption, the broader directive can be dropped to keep linter noise under control.node/full_node_integration_test.go (1)
1-2: Add legacy+buildtag (optional)If you still support Go ≤1.16 tool-chains (e.g. downstream consumers using old IDEs), append the legacy build tag comment:
// +build integrationimmediately above the new
//go:build integrationline.
Not required for Go ≥1.17, but harmless and increases compatibility.node/single_sequencer_test.go (1)
1-2: Consider including legacy build tag for wider compatibilitySame rationale as the previous file – add:
// +build !integrationjust below the modern constraint if Go ≤ 1.16 users must still be able to run
go test.node/single_sequencer_integration_test.go (1)
1-2: Mirror legacy build tag (optional)// +build integrationwill keep pre-1.17 tool-chains happy.
Feel free to ignore if the project policy is “Go ≥ 1.20 only”.scripts/test.mk (1)
31-35: Message copy-paste nit – says “e2e” but runs integration testsThe echo still says “Running e2e tests”; update to reduce confusion.
- @echo "--> Running e2e tests" + @echo "--> Running integration tests".github/workflows/test.yml (1)
80-84: Permission block missing onunit_testjobOther newly added jobs specify minimal
permissions. For parity and to silence GHAS warnings, add:permissions: contents: readunder
unit_test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/test.yml(3 hunks)node/execution_test.go(1 hunks)node/full_node_integration_test.go(1 hunks)node/full_node_test.go(1 hunks)node/helpers.go(1 hunks)node/helpers_test.go(1 hunks)node/single_sequencer_integration_test.go(1 hunks)node/single_sequencer_test.go(1 hunks)pkg/sync/sync_service_test.go(2 hunks)scripts/test.mk(2 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeQL
.github/workflows/test.yml
[warning] 150-150: Unpinned tag for a non-immutable Action in workflow
Unpinned 3rd party Action 'Tests / Code Coverage' step Uses Step uses 'codecov/codecov-action' with ref 'v5.4.3', not a pinned commit hash
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (5)
pkg/sync/sync_service_test.go (1)
82-94: Good call isolating state with a fresh datastoreSwitching to
restartedKVensures the restarted service doesn’t see artefacts from the first run – this makes the restart logic far more deterministic. 👍node/full_node_test.go (1)
1-2: Build tag looks correctThe
//go:build !integrationdirective cleanly splits unit from integration tests and is placed at the required very first line. No issues detected.node/execution_test.go (1)
1-2: Build tag looks correctSame remark as above – the exclusion tag is correctly applied.
scripts/test.mk (1)
10-10: Unit-test target unintentionally enables integration code
go run -tags='run integration'sets BOTH therunandintegrationtags, which contradicts the intent of keeping integration tests out of the unit-test job.
Verify thatscripts/test.goreally needs theintegrationtag. Most likely the unit pipeline should exclude it:- @go run -tags='run integration' scripts/test.go + @go run -tags=run scripts/test.goor pass
-tags='run,!integration'inside the helper script.
Please double-check – failing to exclude the tag will compile integration-only files and defeat the split..github/workflows/test.yml (1)
85-103: Reuse artifact filename constant or path mismatch risk
make test-integration-coverwritescoverage.txtinsidenode/, but the upload step expects./node/coverage.txt. Good now, but fragile if the relativecdchanges.
Consider:path: ${{ github.workspace }}/node/coverage.txtor emit to the workspace root for symmetry with unit tests.
| @echo "--> Generating Code Coverage" | ||
| @go install github.com/ory/go-acc@latest | ||
| @go-acc -o coverage.txt ./... | ||
| @go tool cover -mode=set -o coverage.txt ./... |
There was a problem hiding this comment.
cover target will fail – invalid go tool cover usage
go tool cover does not accept -mode or packages; it only consumes an existing coverage profile.
Current command will exit with “unknown flag: -mode”.
Replace the target with a proper go test invocation that creates the profile:
- @go tool cover -mode=set -o coverage.txt ./...
+ @go test -mod=readonly -failfast -timeout=15m -tags='!integration' \
+ -coverprofile=coverage.txt -covermode=atomic ./...This mirrors what you do for test-integration-cover and restores CI green.
🤖 Prompt for AI Agents
In scripts/test.mk at line 28, the `go tool cover` command is incorrectly used
with the `-mode` flag and package arguments, which it does not support. Replace
this line with a `go test` command that runs tests with coverage enabled and
outputs a coverage profile file, similar to the `test-integration-cover` target.
This will generate a valid coverage profile for use with `go tool cover` and
prevent the command from failing.
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
3 participants
Overview
This pr splits the unit tests from integration in node. this allows the integration tests to have a custom job for integration test as the integration tests were flaky
Summary by CodeRabbit
New Features
Refactor
Chores