Add tests for serve.go pure helpers (#33370)#44929
Conversation
Covers argsToString, getTLSConfig, initLicense, nopPusher.Push, and the three database/migration print warnings. Adds a small captureStdout helper reused across the print tests. No production code changes.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
WalkthroughServe printing helpers were changed to accept an io.Writer and runServeCmd now calls them with os.Stdout. Tests were added in cmd/fleet/serve_test.go: TestArgsToString, TestGetTLSConfig (modern/intermediate), TestInitLicense, and TestPrintMissingMigrationsWarning; test imports were updated to support TLS types, driver.NamedValue, and stdout capture. 🚥 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_test.go`:
- Around line 1478-1498: The captureStdout helper mutates the process-global
os.Stdout without synchronization causing flaky parallel tests; serialize it by
adding a package-level mutex (e.g., captureStdoutMu) and lock it at the start of
captureStdout and unlock only after os.Stdout is restored, so only one test can
swap stdout at a time; also explicitly close the read end (r.Close()) after the
copy goroutine finishes (after <-done) and ensure the write end is closed
(w.Close()) before waiting, keeping the existing helpers and return behavior
intact.
🪄 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: cc219da8-57c9-47b2-9a9a-9379f6eca4a8
📒 Files selected for processing (1)
cmd/fleet/serve_test.go
Adds a package-level mutex around the os.Stdout swap so concurrent test use is safe, and explicitly closes the read end of os.Pipe to avoid a file-descriptor leak.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #44929 +/- ##
==========================================
+ Coverage 66.73% 66.85% +0.12%
==========================================
Files 2684 2689 +5
Lines 216067 217154 +1087
Branches 10066 10066
==========================================
+ Hits 144190 145180 +990
- Misses 58768 58807 +39
- Partials 13109 13167 +58
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:
|
|
@lucasmrod - Can I please get a review on this PR? Thanks! 🙏 🙇 |
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 56072
1.
|
| Test | Value | Rationale |
|---|---|---|
TestGetTLSConfig |
✅ High | Security-critical — validates exact cipher suites, min TLS version, and curve preferences |
TestInitLicense |
✅ High | Real branching logic (dev / expired / no-key), meaningful invariants |
TestArgsToString |
✅ Medium-High | Documents boundary cases (nil, named, positional, mixed types) of a utility formatter |
TestPrintMissingMigrationsWarning |
Worth keeping for the tablesAndDataToString branch coverage (tables-only, data-only, both, neither) |
|
TestPrintDatabaseNotInitializedError |
Just asserts static substrings from a fmt.Printf with no branching logic |
|
TestPrintFleetv4732FixNeededMessage |
Same — the function is a single static fmt.Printf, nothing to go wrong |
|
TestNopPusher |
❌ Very Low | nopPusher.Push unconditionally returns nil, nil — there is no logic to verify |
The two low-value print tests and TestNopPusher could be dropped without meaningfully reducing safety. The effort would be better spent on the io.Writer refactor, which would also make TestPrintMissingMigrationsWarning (the one with actual branches) both simpler and parallelizable.
|
@raju249 please address the Code Rabbit comment above. |
Per review feedback, narrowed this PR's scope to test-only additions for the high-value pure helpers. Removed TestNopPusher and the three print-function tests along with the captureStdout helper they relied on. The print-function refactor to accept io.Writer for proper test injection will land in a separate follow-up PR.
Make the three database-state print functions take an io.Writer so tests can pass a *bytes.Buffer instead of mutating os.Stdout globally. Production callers in runServeCmd pass os.Stdout. The migrations-warning test is restored under this pattern, and all four added tests now run with t.Parallel() — which the previous captureStdout/mutex approach could not support.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/fleet/serve_test.go (1)
1406-1445: ⚡ Quick winConsider
assert.Equalinstead ofassert.ElementsMatchfor order-sensitive slices.The test uses
ElementsMatchforCurvePreferencesandCipherSuites, which ignores order. However, the order of these slices determines TLS preference order whenPreferServerCipherSuitesis true, making it security-critical. Usingassert.Equalwould catch accidental reordering bugs.🔒 Suggested hardening for TLS config validation
- assert.ElementsMatch(t, expectedCurves, cfg.CurvePreferences) - assert.ElementsMatch(t, []uint16{ + assert.Equal(t, expectedCurves, cfg.CurvePreferences) + assert.Equal(t, []uint16{ tls.TLS_AES_128_GCM_SHA256, tls.TLS_AES_256_GCM_SHA384, tls.TLS_CHACHA20_POLY1305_SHA256, tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, }, cfg.CipherSuites)Apply the same change to the "intermediate" subtest.
🤖 Prompt for 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. In `@cmd/fleet/serve_test.go` around lines 1406 - 1445, The test TestGetTLSConfig uses assert.ElementsMatch for order-sensitive fields which hides regressions in preference order; change the assertions in both subtests to use assert.Equal on cfg.CurvePreferences and cfg.CipherSuites (and keep the existing checks for cfg.MinVersion and cfg.PreferServerCipherSuites) so the test compares order as well as content when calling getTLSConfig; update assertions in the "modern" and "intermediate" subtests to assert.Equal against the expected ordered slices.
🤖 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.
Nitpick comments:
In `@cmd/fleet/serve_test.go`:
- Around line 1406-1445: The test TestGetTLSConfig uses assert.ElementsMatch for
order-sensitive fields which hides regressions in preference order; change the
assertions in both subtests to use assert.Equal on cfg.CurvePreferences and
cfg.CipherSuites (and keep the existing checks for cfg.MinVersion and
cfg.PreferServerCipherSuites) so the test compares order as well as content when
calling getTLSConfig; update assertions in the "modern" and "intermediate"
subtests to assert.Equal against the expected ordered slices.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e7e87c7a-af40-46ad-902c-e2c2b50ae4e2
📒 Files selected for processing (2)
cmd/fleet/serve.gocmd/fleet/serve_test.go
|
@coderabbitai - Can you take a review after your last suggestion? |
|
Let me trigger a fresh review of the current state of the PR. ✅ Actions performedFull review triggered. |
|
@getvictor - I think this is ready for another round of review. Addressed suggestions from coderabbit. Can you take a look, please? Thanks! 🙏 🙇 |
getvictor
left a comment
getvictor
left a comment
There was a problem hiding this comment.
Looks good overall. Just a couple more small changes. Thank you.
Revert printDatabaseNotInitializedError and printFleetv4732FixNeededMessage to their original parameter-less signatures since they have no test coverage. Keep printMissingMigrationsWarning's io.Writer parameter, which the test exercises. Drop t.Parallel from subtests per review — outer-level parallel coverage is sufficient and avoids the per-subtest overhead.
|
Thanks for the review @getvictor This is ready for another round of review. Thanks! 🙏 🙇 |
Extracts the OTEL trace, metric, and log provider setup out of `runServeCmd` and into `initOTELProviders` in a new `cmd/fleet/otel.go`. Same pattern as the prior extractions on this issue (#44929, #45343, #45583, #46166). Side effects (`otel.SetTracerProvider`, `otel.SetMeterProvider`) are preserved inside the extracted function, so runtime behavior is identical. Three unit tests in `cmd/fleet/otel_test.go`: - OTEL disabled (the common production path) returns `(nil, nil, nil)` and never calls `initFatal`. - OTEL enabled without log export returns non-nil trace and meter providers; logger provider stays nil. - Log export enabled returns all three providers non-nil. One honest note on coverage: the four `initFatal` sites inside the function are paranoid wrapping for OTEL SDK constructors that don't dial at construction time, so the error paths are hard to drive in tests without mocking the SDK. The tests above exercise the success paths and the disabled gate, which is the bulk of the realistic flow. This continues the path toward `serve.go` >60% coverage per the discussion on #33370 — `serve.go` is now ~100 lines shorter and the OTEL phase is testable as a unit. Remaining slices per the broader plan: MDM Apple init, datastore init, Redis init. **Related issue:** Refs #33370 # Checklist for submitter - [x] Added/updated automated tests - Changes file: not applicable — internal refactor with no user-visible behavior change <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactor** * Centralized OpenTelemetry provider initialization into a single setup path, simplifying startup and shutdown behavior and making observability configuration clearer. * **Tests** * Added unit tests covering disabled/enabled telemetry paths and optional log export, plus cleanup logic to ensure providers are shut down correctly. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/fleetdm/fleet/pull/46421?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
](https://app.coderabbit.ai/change-stack/fleetdm/fleet/pull/46421?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
2 participants
First PR in the staged plan from #33370. Adds unit tests for several testable helpers in
cmd/fleet/serve.go— argument stringification, TLS profile config, license initialization, and the missing-migrations warning.The migrations-warning test required threading
io.WriterthroughprintMissingMigrationsWarningso it can pass*bytes.Bufferinstead of mutatingos.Stdout. The other two database-state print functions stay as-is since they aren't tested in this PR.Related issue: Part of #33370 (intentionally not using auto-close keywords since this is the first of multiple PRs against this issue).
Checklist for submitter
If some of the following don't apply, delete the relevant line.
Testing
Database migrations
N/A — no database migrations in this PR.
New Fleet configuration settings
N/A — no new configuration settings.
fleetd/orbit/Fleet Desktop
N/A — no agent code changes.