Fix: Follow-up review findings from #1076/#1080 - #1107
Conversation
- Spec doc bullet 7 said "not yet on this branch's own tree until it merges" about #1079's TimeoutStopSec fix — accurate when written on an unmerged branch, but both #1079 and #1080 are now merged into main, so the sentence inverted: it read as "hasn't merged yet" about something that has. Dropped the merge-status framing; the symbol citation still stands on its own. - unloadService's "disable --now failure is reported" test only checked strings.Contains(err.Error(), "disable"), so a wrong unit name would still pass. Pinned the exact command string ("--user disable --now cortex.service"), matching the sibling assertion in TestControlService_Linux. - TestLingerEnabled hardcoded uid "501" (macOS-shaped) in a Linux-only test, inconsistent with the rest of the file's strconv.Itoa(os.Getuid()). Replaced it, and added a subtest pinning the exact show-user invocation (uid + --property=Linger) — the existing cases only checked the parsed result, so a wrong property flag or wrong uid would have stayed green. Found during review of #1076/#1080; both explicitly called out as real, not the two other findings from the same round that were noted as out of scope (tracked in #1106 instead). Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Alan Cha <Alan.cha1@ibm.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe changes tighten systemd lifecycle test assertions, use the current process uid in linger tests, add a loginctl call-count assertion, and revise a design-document reference to the ChangesLinux systemd lifecycle
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable merge-blocking risk remains. The update improves test coverage for existing lifecycle behavior without changing runtime behavior. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 1 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
huang195
left a comment
huang195
left a comment
There was a problem hiding this comment.
Tight, accurate follow-up — three test/doc fixes, every claim verifiable against source.
Verified (fetched from fix/systemd-followup-945 and main, not a local checkout):
| Claim | Result |
|---|---|
unloadService error string |
cmd_service_platform.go:296 → "systemctl --user disable --now %s: ..." with systemdUnit = "cortex.service" (cmd_service.go:31) — substring assert matches exactly |
lingerEnabled invocation |
cmd_service_platform.go:270 → loginctl show-user <uid> --property=Linger — new want string matches byte-for-byte |
| uid mirrors production | cmd_service_platform.go:244 derives uid the same way; this file already did so at line 275, so the change is locally consistent too |
| Doc rewording | #1079 is merged; TimeoutStopSec=20 is on main at cmd_service_platform.go:150, asserted at cmd_service_test.go:87. The revised wording is correct |
| Deferred findings | #1106 is open and covers both |
The unloadService assertion now matches its TestControlService_Linux sibling exactly, and the new show-user subtest closes a real hole: the four table cases never checked what loginctl was asked, since fakeLoginctl answers identically regardless of its arguments. Out-of-scope items were deferred to #1106 rather than smuggled in — appreciated.
One nit inline, non-blocking.
Areas reviewed: Go tests, Docs · Agent/IDE config (.claude/.vscode): none · Secrets scan: clean · Commits: 1, signed off · CI: all green (27 checks)
| t.Run("invokes show-user with the exact uid and property", func(t *testing.T) { | ||
| logPath, logLine := callLog(t) | ||
| fakeLoginctl(t, "#!/bin/sh\n"+logLine+"\necho 'Linger=yes'\nexit 0\n") | ||
| lingerEnabled(uid) |
There was a problem hiding this comment.
nit: the return value is discarded here, so the stub's echo 'Linger=yes' is inert — this subtest passes identically with a bare exit 0 body. The sibling invocation-pinning subtests in this file (TestControlService_Linux, the tc.action loop) do check what came back.
A one-liner would keep the canned output load-bearing and match that style:
if !lingerEnabled(uid) {
t.Error("lingerEnabled() = false, want true for Linger=yes")
}The explicit yes table case already covers the parse, so this is belt-and-braces — take it or leave it.
3 participants
Summary
TimeoutStopSecfix was "not yet on this branch's own tree until it merges," which read backwards now that both Fix: Set an explicit TimeoutStopSec on the Linux service unit #1079 and Test: Add a fakeSystemctl/fakeLoginctl harness for the Linux service path #1080 are merged intomain.unloadService'sdisable --nowfailure test to the exact command string, matching its sibling inTestControlService_Linux— previously a wrong unit name would still have passed.TestLingerEnabled's hardcoded macOS-shaped uid literal ("501") in a Linux-only test, and adds a subtest pinning the exactshow-user <uid> --property=Lingerinvocation — the existing cases only checked the parsed result, so a wrong property flag or uid would have stayed green.Context
Three genuine findings from the post-merge review pass on #1076/#1080, called out by @huang195. Two other findings from the same round (
serviceIsCurrent/reportInstallSuccessstill branching ongoosinternally, and a latentcallLogzero-argument edge case) were explicitly noted as out of scope there — tracked separately in #1106 instead of included here.Test plan
go build,go vet,gofmtcleanTestUnloadService_LinuxandTestLingerEnabledpass, including the new subtestcmd/abctlsuite passes (one pre-existing, unrelated local-machine failure excluded)Assisted-By: Claude (Anthropic AI) noreply@anthropic.com
Summary by CodeRabbit