Add regression tests documenting alerter defects - #410
Open
dpage wants to merge 2 commits into
Open
Conversation
Verify ten claimed defects in the alerting subsystem against a live PostgreSQL instance using the existing integration-test harness. No production code is changed; these tests record current behaviour so that fixes have a baseline to break. Two styles are used. TestAudit* tests assert the current, defective behaviour with a doc comment stating what the behaviour should be, so they pass in CI and fail once a fix lands. TestAudit*Demo tests assert the correct behaviour and therefore fail today; they are skipped unless ALERTER_DEFECT_DEMO=1 so that CI stays green. Covers the metric_staleness fire and clear loop, the missing pg_stat_archiver table, the hardcoded transaction wraparound metric, the pg_settings one hour window, the NULL CPU column on Linux, the anomaly baseline ordering bug, the fallback baseline warmup gate, the unreduced cache hit ratio query, the slow query lifetime average, and the ignored database name in anomaly detection. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L2VcS2NfYNkagXRo7khohq
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdded integration-test suites for database metric defects, alert lifecycle defects, cache-hit behavior, and anomaly baseline and candidate handling. The suites seed representative schemas and data, capture notifications, and include opt-in corrected-behavior demos. ChangesAudit defect integration tests
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 271 |
| Duplication | 36 |
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.
![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
🧹 Nitpick comments (3)
alerter/src/internal/database/audit_defects_test.go (2)
728-742: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the hardcoded registry counts with a name-based assertion.
The test pins
len(metricRegistry) == 32andlen(empty) == 14. Any new metric added to the registry fails this test even when no defect is introduced, and the failure message does not tell the author which metric changed. The test already collectsemptyas a sorted name list, so assert on that list instead.Keep the audit claim documented in the comment, and compare the set of metric names that lack
historicalSQL.♻️ Proposed refactor to a name-set assertion
- // Pinned figures for the current code. The audit's "12 of 34" is - // wrong on both numbers; 34 is the count of seeded alert_rules - // rows, not of registry metrics. - const ( - wantRegistryEntries = 32 - wantEmptyHistorical = 14 - ) - if len(metricRegistry) != wantRegistryEntries { - t.Errorf("registry entries = %d, want %d", - len(metricRegistry), wantRegistryEntries) - } - if len(empty) != wantEmptyHistorical { - t.Errorf("entries with empty historicalSQL = %d, want %d", - len(empty), wantEmptyHistorical) - } + // Pinned set for the current code. The audit's "12 of 34" is wrong + // on both numbers; 34 is the count of seeded alert_rules rows, not + // of registry metrics. Pinning names rather than counts reports + // exactly which metric changed when this test fails. + wantEmpty := []string{ + // TODO: fill in from the t.Logf output above. + } + if !slices.Equal(empty, wantEmpty) { + t.Errorf("metrics with empty historicalSQL:\n got: %v\nwant: %v", + empty, wantEmpty) + }Add
"slices"to the import block.🤖 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 `@alerter/src/internal/database/audit_defects_test.go` around lines 728 - 742, Replace the count-based assertions and wantRegistryEntries/wantEmptyHistorical constants in the audit test with a sorted name-set assertion on the existing empty list. Define the expected metric names lacking historicalSQL, import and use slices comparison as needed, and retain the audit-claim comment while making failures identify the differing names.
744-755: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the empty-
historicalSQLerror
GetHistoricalMetricValuesreturnshistorical data not implemented for metric %sbefore it executes SQL. Assert this error text instead of only checkingerr != nil, so the test detects a removed guard or another SQL error.🤖 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 `@alerter/src/internal/database/audit_defects_test.go` around lines 744 - 755, Update the GetHistoricalMetricValues assertion in the empty-metric loop to verify the exact expected “historical data not implemented for metric %s” error text for each name, rather than only checking that an error occurred. Preserve the existing failure message context and fallback-path coverage.alerter/src/internal/engine/audit_defects_test.go (1)
626-634: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsolidate the two
metrics.pg_stat_databasefixture definitions.Line 503 creates
metrics.pg_stat_databaseinline withblks_hitandblks_read. This constant creates the same table withdeadlocks. Two divergent definitions of one fixture table are easy to break. Define one constant that carries all columns used by the audit tests, and reuse it in both places.♻️ Proposed single fixture definition
createStatDatabaseTableSQL = ` CREATE TABLE metrics.pg_stat_database ( connection_id INTEGER NOT NULL, database_name VARCHAR(255) NOT NULL, datname TEXT, + blks_hit BIGINT, + blks_read BIGINT, deadlocks BIGINT, collected_at TIMESTAMPTZ NOT NULL ) `Then replace the inline
CREATE TABLEat Line 503 withpool.Exec(ctx, createStatDatabaseTableSQL).🤖 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 `@alerter/src/internal/engine/audit_defects_test.go` around lines 626 - 634, Consolidate the duplicate metrics.pg_stat_database fixture definitions by expanding createStatDatabaseTableSQL to include every column used by the audit tests, including blks_hit, blks_read, and deadlocks. Replace the inline CREATE TABLE statement near the other setup with pool.Exec(ctx, createStatDatabaseTableSQL), leaving both setup paths on the shared constant.
🤖 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 `@alerter/src/internal/database/audit_defects_test.go`:
- Around line 852-858: Update the test assertion around gotFirstViolates so it
no longer assumes the database’s returned row order: first verify that exactly
one collected value violates seededThreshold, then sort values by CollectedAt
and assert the intended oldest-interval position using the sorted slice.
Preserve the existing evaluator and cleaner behavior checks, and document the
ordering dependency only if wire-order dependence is intentionally being tested.
---
Nitpick comments:
In `@alerter/src/internal/database/audit_defects_test.go`:
- Around line 728-742: Replace the count-based assertions and
wantRegistryEntries/wantEmptyHistorical constants in the audit test with a
sorted name-set assertion on the existing empty list. Define the expected metric
names lacking historicalSQL, import and use slices comparison as needed, and
retain the audit-claim comment while making failures identify the differing
names.
- Around line 744-755: Update the GetHistoricalMetricValues assertion in the
empty-metric loop to verify the exact expected “historical data not implemented
for metric %s” error text for each name, rather than only checking that an error
occurred. Preserve the existing failure message context and fallback-path
coverage.
In `@alerter/src/internal/engine/audit_defects_test.go`:
- Around line 626-634: Consolidate the duplicate metrics.pg_stat_database
fixture definitions by expanding createStatDatabaseTableSQL to include every
column used by the audit tests, including blks_hit, blks_read, and deadlocks.
Replace the inline CREATE TABLE statement near the other setup with
pool.Exec(ctx, createStatDatabaseTableSQL), leaving both setup paths on the
shared constant.
🪄 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: 915da777-e90c-4c11-99ee-f0872060b7ff
📒 Files selected for processing (2)
alerter/src/internal/database/audit_defects_test.goalerter/src/internal/engine/audit_defects_test.go
Address a Codacy critical finding and CodeRabbit review comments on the regression tests. No production code changes. Codacy's Opengrep engine flagged go_sql_rule-concat-sqli (CWE-89) at two sites where a string literal was concatenated with a table-driven test field and the result reached a pool.Exec argument. Every value is bound as a parameter, so the finding is a false positive of the taint rule, but the pattern is avoidable: the connection name is now carried in the test table as a literal rather than built by concatenation. Replace the positional assertion on the cache hit ratio rows. The metric query carries no ORDER BY, so asserting on the first row as returned relied on a plan-dependent wire order and could fail without any code change. The test now asserts the structural defect on the rows as returned, then sorts a copy by collected_at before any positional check. The production cleaner's dependence on that unspecified order is the defect being recorded, not an assumption of the test. Replace the pinned registry counts with a sorted list of the metric names that lack historical SQL, so a registry change names the metric that moved instead of reporting a bare count mismatch. Assert the exact "historical data not implemented" error text per metric rather than only a non-nil error, so a removed guard or an unrelated SQL error is detected. Consolidate the two divergent metrics.pg_stat_database fixture definitions into one constant carrying every column the audit tests use. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L2VcS2NfYNkagXRo7khohq
dpage
added a commit
that referenced
this pull request
Aug 12, 2026
PR #410 adds audit_defects_test.go to the same package with fixtures called insertStalenessRuleSQL, insertProbeConfigSQL, and friends, so the two files would have collided on those identifiers once both landed. Prefixing the fixtures here with 'staleness' keeps the two sets apart and lets them coexist without either side needing edits at merge time.
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
2 participants
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.
Summary
Adds test-only coverage that empirically verifies ten claimed defects
in the alerting subsystem. No production code is changed; the tests
establish a baseline so that each fix has something concrete to break.
This came out of a review of dashboard charts showing cumulative
rather than point-in-time statistics. The chart findings are tracked in
#400, #401, #402, #403 and #404. The alerter findings verified here are
tracked in #405, #406, #407, #408 and #409.
Two test styles
TestAudit*tests assert the current, defective behaviour with a doccomment stating what the behaviour should be. They pass in CI today
and fail once a fix lands, which is the signal to update them
alongside the fix.
TestAudit*Demotests assert the correct behaviour and therefore failagainst current code. They are skipped unless
ALERTER_DEFECT_DEMO=1so CI stays green. Four exist, covering the staleness loop, baseline
ordering, the fallback baseline warmup gate, and the slow query
lifetime average.
Running the demo tests produces:
What was verified
Confirmed: the
metric_stalenessfire and clear loop and its missingcooldown guard (#405); the missing
metrics.pg_stat_archivertable,the hardcoded
transaction_wraparoundmetric, thepg_settingsonehour window, and the NULL CPU column consequence on Linux (#406); the
slow_query_countlifetime average (#407); the anomaly baselineordering bug and the ignored
database_name(#408).Partially confirmed, with corrections to the original claims:
The fallback baseline mechanism is real, but the count was wrong.
The registry has 32 entries with 14 empty
historicalSQL, not 12 of34; 34 is the number of seeded alert rules, not registry metrics.
cache_hit_ratio_lowhas two failure modes rather than one. Aviolation in the newest interval flaps; a violation in the oldest
interval latches, because the cleaner breaks on the first row of an
unordered result.
Not provable here: that
system_statsleavesprocessor_time_percentNULL on Linux. The extension cannot be installed in this environment,
so the premise is confirmed from the upstream C source while the
consequence is covered by a table-driven test.
Testing
Tests use the existing integration harness (
TEST_AI_WORKBENCH_SERVER,NewTestDatastore,newEngineSpockTestEnv,newDetectAnomaliesEnv)against a live PostgreSQL 16 instance. The full alerter suite passes
under
-race -p=1.One caveat to flag:
make test-allfails atlintfor a pre-existingenvironmental reason, reproduced on a clean tree with no changes:
golangci-lint 2.5.0is built withgo1.25.1while the modules targetgo 1.26.1. The linter cannot run in this environment at all; this isunrelated to the change but means lint has not been verified.
The project's 90% coverage floor applies to new and modified
production code. This change adds no production code.
🤖 Generated with Claude Code
https://claude.ai/code/session_01L2VcS2NfYNkagXRo7khohq
Generated by Claude Code
Summary by CodeRabbit