Remove dead code that needed test edits rather than deletions - #397
Open
dpage wants to merge 1 commit into
Open
Conversation
The third and final pass over the reachability findings. The earlier
mechanical pass deferred 42 functions because deleting them broke tests
that also cover live code, so those needed the tests editing rather than
removing. Working through them individually, 18 turned out to be
genuinely removable and the rest did not.
The largest group is the alerter's notification-channel write API: ten
methods covering channel create, update and delete, email recipients,
connection links, notification history and reminder state, none of which
has a caller. The alerter reads notification channels; the server owns
every write. Those went along with the five integration tests that
existed solely to exercise them, and their assertions in the shared
closed-pool error-path test were excised individually so the remaining
assertions still run.
The rest are a datastore pool accessor, a monitored-pool max-connections
getter, two compaction analytics reporters, a probe-availability lookup,
and three session tracing helpers.
Twenty-four of the 42 are deliberately left in place, because deleting
them would have meant weakening tests that exercise live code:
- Injection seams that exist so tests can substitute fakes:
AlertHandler.setAlertResolver and setUnacknowledgeFn,
newVisibilityListerWithSource, NewWebhookNotifierAllowInternal
(whose sibling's doc comment explicitly points at it for localhost
testing), and overview.NewHandler.
- Accessors their own tests depend on: CompactionCache.Clear and
ReloadableConfig.Get, both of which had tests that silently inverted
when the call was excised, plus Hub.Count and
GetAnomalyCandidateByID, which verifies a live write path.
- The logging level setters SetLevel and SetLogLevel. Both packages
default to a level that suppresses output, so their tests need the
setter to exercise filtering of live logging code at all.
- MessageClass.String, a Stringer used for test diagnostics.
- AuthHandler.Close, which stops h.totalRateLimiter and is called by
nothing in cmd/mcp-server. That looks like a leaked goroutine in
production rather than dead code, and wants fixing rather than
deleting; noted for a follow-up.
Four more are blocked until the earlier branch merges, because the only
remaining callers are themselves being deleted there:
IsAPITokenFromContext (called from access.go), Logger.Trace (from
LogQueryTrace) and cron's Parser.Validate (from the package-level
Validate).
Coverage: collector 85.6% (from 85.5%), server 55.5% (unchanged),
alerter 64.4% (from 65.9%). The alerter drop is the arithmetic of
removing well-covered code, and it stays above the 60% repository goal.
The only failing tests are the two pre-existing vector(3) fixture
failures from #337, which reproduce identically on unmodified main.
WalkthroughThe change removes unused APIs from alerter database access, collector pool management, compactor analytics, and tracing. It also removes dedicated tests, adjusts remaining integration assertions, and documents the removals in the changelog. ChangesUnused API removal
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 `@docs/changelog.md`:
- Around line 258-266: Update the removed-function count in the changelog entry
from 18 to 17 so it matches the listed groups: 10 notification-channel methods,
1 pool accessor, 2 analytics reporters, 1 probe lookup, and 3 tracing helpers.
🪄 Autofix
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: f0355baa-ab05-46ee-9d85-05dc7b99f8c9
📒 Files selected for processing (14)
alerter/src/internal/database/datastore.goalerter/src/internal/database/datastore_test.goalerter/src/internal/database/error_paths_integration_test.goalerter/src/internal/database/notification_queries.goalerter/src/internal/database/notification_queries_full_integration_test.goalerter/src/internal/database/queries.goalerter/src/internal/database/queries_full_integration_test.gocollector/src/database/monitored_pool.gocollector/src/database/monitored_pool_test.godocs/changelog.mdserver/src/internal/compactor/analytics.goserver/src/internal/compactor/analytics_test.goserver/src/internal/tracing/tracer.goserver/src/internal/tracing/tracer_test.go
💤 Files with no reviewable changes (13)
- collector/src/database/monitored_pool.go
- alerter/src/internal/database/datastore_test.go
- server/src/internal/tracing/tracer_test.go
- alerter/src/internal/database/queries.go
- server/src/internal/tracing/tracer.go
- alerter/src/internal/database/datastore.go
- collector/src/database/monitored_pool_test.go
- alerter/src/internal/database/notification_queries.go
- alerter/src/internal/database/queries_full_integration_test.go
- server/src/internal/compactor/analytics.go
- alerter/src/internal/database/error_paths_integration_test.go
- server/src/internal/compactor/analytics_test.go
- alerter/src/internal/database/notification_queries_full_integration_test.go
Comment on lines
+258
to
+266
| - Remove a further 18 unused functions across the collector, server, and | ||
| alerter, together with the tests that existed only to exercise them. | ||
| The largest group is the alerter's notification-channel write API, | ||
| where ten methods covering channel creation, updates, deletion, email | ||
| recipients, connection links, notification history, and reminder state | ||
| had no caller; the alerter reads notification channels, whilst the | ||
| server owns every write. The rest are a pool accessor, two compaction | ||
| analytics reporters, a probe-availability lookup, and three session | ||
| tracing helpers. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the removed-function count.
The listed groups total 17 functions: 10 notification-channel methods, 1 pool accessor, 2 analytics reporters, 1 probe lookup, and 3 tracing helpers. Change 18 to 17, or add the missing function to the list.
🤖 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 `@docs/changelog.md` around lines 258 - 266, Update the removed-function count
in the changelog entry from 18 to 17 so it matches the listed groups: 10
notification-channel methods, 1 pool accessor, 2 analytics reporters, 1 probe
lookup, and 3 tracing helpers.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | -46 |
| Duplication | -4 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
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.
The third and final pass over the reachability findings, completing the
set alongside #395 and #396.
#395 deliberately deferred 42 functions because deleting them broke tests
that also cover live code, so each needed its tests edited rather
than removed. Working through them one at a time, 18 turned out to be
genuinely removable and 24 did not, which is the most useful result
here: the deferral list was not a backlog of easy wins.
What was removed
The largest group is the alerter's notification-channel write API:
ten methods covering channel create/update/delete, email recipients,
connection links, notification history and reminder state, none with a
caller. The alerter reads notification channels; the server owns every
write. Those went with the five integration tests that existed solely to
exercise them, and their assertions in the shared closed-pool error-path
test were excised individually so the remaining assertions still run.
The rest: a datastore pool accessor, a monitored-pool max-connections
getter, two compaction analytics reporters, a probe-availability lookup,
and three session tracing helpers.
What was deliberately kept, and why
This is the substantive half. Twenty-four were left in place because
removing them would mean weakening tests of live code:
AlertHandler.setAlertResolverandsetUnacknowledgeFn,newVisibilityListerWithSource,overview.NewHandler, andNewWebhookNotifierAllowInternal— whose sibling's doc commentexplicitly says "Use NewWebhookNotifierAllowInternal for testing
against localhost endpoints".
CompactionCache.ClearandReloadableConfig.Get. Both are worth singling out, because excisingthe call left tests that still compiled, still linted clean, and
silently failed: without
Clear(),TestCompactionCache_Sizeasserts a size of 0 against a cache holding two entries. Also
Hub.CountandGetAnomalyCandidateByID, the latter being how a testverifies the live
CreateAnomalyCandidateround-trips.SetLevelandSetLogLevel.internal/loggingdefaults toLevelErrorand the database logger toLogLevelNone, so without the setter their tests cannot exercisefiltering of live logging code at all.
MessageClass.String, a Stringer used for test diagnostics;removing it degrades failure messages to bare integers.
AuthHandler.Close, which stopsh.totalRateLimiterand is calledby nothing in
cmd/mcp-server. That reads as a leaked tickergoroutine in production rather than dead code, so it wants wiring up
rather than deleting. Flagged for a follow-up.
Four more are blocked until #395 merges, because their only remaining
callers are themselves deleted there:
IsAPITokenFromContext(calledfrom
access.go),Logger.Trace(fromLogQueryTrace), and cron'sParser.Validate(from the package-levelValidate).Verification
gofmtclean,golangci-lint0 issues on all three modules,go vetclean.-race -p=1. The only failures are the twopre-existing
vector(3)fixture failures from Gemini provider: (1) knowledge base search silently falls back to OpenAI due to missing gemini_embedding column in search_knowledgebase.go; (2) session startup fails with 400 "empty Part" error #337, which reproduceidentically on unmodified
main.gofmt -e, after an earlierautomated attempt corrupted a backtick-quoted SQL literal by excising
lines inside it. That is fixed, and the parse check is why I trust the
rest.
mainThe alerter drop is the arithmetic of removing well-covered code: 193
production lines and 344 test lines of notification CRUD that were
thoroughly tested. It stays above the 60% repository goal, and no new
code is added so the 90% floor for new and modified code does not apply.
This branch is cut from
mainso CI runs against it, but it conflictswith #395 and must be rebased after that merges. I verified this with a
trial merge; the conflicts are in:
All are cases where #395 removed one function from a file and this PR
removes another. There is no conflict with #396. Happy to do the
rebase once #395 lands.
Summary by CodeRabbit
Removed
Documentation