Boot-test runServeCmd end to end (serve.go ~7% to ~64% coverage)#48261
Open
raju249 wants to merge 4 commits into
Open
Boot-test runServeCmd end to end (serve.go ~7% to ~64% coverage)#48261raju249 wants to merge 4 commits into
raju249 wants to merge 4 commits into
Conversation
runServeCmd was untested (serve.go was ~7% covered) because it blocks on an OS signal and wires the whole server together. Add a gated integration test that boots the full server against a real test MySQL and Redis, waits for /healthz, and shuts it down gracefully, taking serve.go to ~64% and runServeCmd from 0% to ~62%. To let the test trigger a graceful shutdown without sending a process signal, the shutdown select now also watches cmd.Context().Done(). This is inert in production: the root command runs via Execute (not ExecuteContext), so cmd.Context() is context.Background() and never cancels; only tests run the command with a cancelable context. The test covers two paths: a full boot with Apple MDM enabled (a server private key brings up the MDM protocol services and SCEP setup), and a fail-fast path where an invalid Redis host-cache config aborts startup through initFatal. Both are gated behind MYSQL_TEST and REDIS_TEST. Refs fleetdm#33370
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe serve command now treats command-context cancellation as a shutdown trigger in addition to OS signals and fatal database errors. A new end-to-end test file boots the server with MySQL and Redis test infrastructure, configures boot-time environment variables, polls Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/fleet/serve_boot_test.go`:
- Around line 20-24: The boot test helpers and scenarios are still mutating
process-wide state after the parent test is parallelized, including env changes
in setBootEnv and swapping the package-global initFatal, which can leak across
other tests. Update the serve boot test flow to avoid global mutation by using a
non-parallel DB setup for this e2e test or by threading config through command
args/config instead of env, and eliminate the initFatal replacement in the
relevant boot scenario helpers.
- Around line 109-117: The health-check polling in waitHealthy uses the default
http.Get client, which can hang on a stalled connection and outlive
assert.Eventually’s timeout. Update waitHealthy to use an http.Client with a
bounded Timeout for the GET request to /healthz, and keep the retry loop logic
unchanged so the health check still polls until the server becomes ready or the
overall wait expires.
- Around line 155-163: The serve boot test currently cancels the context only
after the health/assertion checks, so a failure in those assertions can skip
shutdown and leave the server goroutine/listener running. In the `Test...` flow
around `runServe`, `waitHealthy`, and `waitShutdown`, defer the `cancel()`
immediately after creating the context (before `require.Emptyf`/`require.True`)
so cleanup always runs even if an assertion aborts the subtest.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 525277e2-02d6-4359-be2a-c51bab722efc
📒 Files selected for processing (2)
cmd/fleet/serve.gocmd/fleet/serve_boot_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #48261 +/- ##
==========================================
+ Coverage 67.30% 67.79% +0.48%
==========================================
Files 3660 3661 +1
Lines 231442 231509 +67
Branches 12155 12155
==========================================
+ Hits 155762 156940 +1178
+ Misses 61742 60480 -1262
- Partials 13938 14089 +151
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- Load the schema directly instead of via CreateMySQLDS so the test is not marked parallel. It mutates process-global state (config env and the initFatal var), which must not race with other package tests; with the test serial these are safe (verified with go test -race), and env is set via t.Setenv so it auto-restores. - Give the /healthz client a bounded timeout so a stalled connection can't outlive the poll window. - Defer the context cancel and shutdown wait so the server is always torn down even if an assertion aborts the subtest.
- Use require.NoError for the shutdown error assertion (testifylint). - Use t.Context() for the fail-fast error-path scenario, which needs no manual cancellation (modernize).
Contributor
Author
|
Hello @getvictor @MagnusHJensen - This one MR adds tests for the runServeCmd. Improves the coverage with happy path and a few non happy path tests. Would you mind taking a look, please? Let me know what do you think! Thanks! |
Fleet's custom gocritic rule disallows constructing http.Client directly; use fleethttp.NewClient with a timeout option instead.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds an end-to-end boot test for
runServeCmd, the main server entry point. This is the coverage milestone for #33370:serve.gogoes from ~7% to ~64%, andrunServeCmditself from 0% to ~62%.The earlier PRs on this issue (#44929, #45343, #45583, #46166, #46421, #46517, #46742, #46830, #46893, #47151, #47562, #47891) extracted testable pieces out of
runServeCmd, but the function itself stayed at 0% — it blocks on an OS signal and wires the entire server together, so the only way to cover it is to actually boot it. This PR does that.TestRunServeCmd(gated behindMYSQL_TEST+REDIS_TEST) boots the full server against a real migrated test MySQL and Redis, waits for/healthz, then cancels the command context to trigger a graceful shutdown. It covers two paths:initFataland returns rather than serving, covering the Redis-init error path and the nil-pool guard.Beyond coverage, this doubles as a regression net for the ongoing
runServeCmdslicing: a future change that breaks startup now fails this test instead of reaching a release.One production change, in
runServeCmd's shutdownselect: it now also watchescmd.Context().Done(). This is inert in production — the root command runs viaExecute()(notExecuteContext()), socmd.Context()iscontext.Background()and never cancels. Only the test runs the command with a cancelable context, which is how it shuts the server down without sending a real signal (aSIGTERMwould kill the test binary).A couple of notes for reviewers:
os.Setenv(nott.Setenv) because the MySQL test helper marks the test parallel; the boot scenarios run as serial subtests so the process-global config env doesn't race.runServeCmdregisters metrics with the process-global Prometheus registry, which can only happen once per process, so there is a single full boot here; the error-path scenario fails before that registration.FLEET_UPGRADES_ALLOW_MISSING_MIGRATIONS=1.It adds ~2s to the
cmd/fleet(main) test bundle, which is well off the CI critical path.Related issue: Refs #33370
Checklist for submitter
Summary by CodeRabbit