amtool: add --test-file flag to config routes test for batch routing and inhibition testing#5175
Open
alliasgher wants to merge 2 commits into
Open
amtool: add --test-file flag to config routes test for batch routing and inhibition testing#5175alliasgher wants to merge 2 commits into
alliasgher wants to merge 2 commits into
Conversation
Add `--test-file` flag to `amtool config routes test` that accepts a
YAML file containing named test cases. Each test case fires a set of
alerts together and asserts per-alert expectations:
expected_receivers: [list] – route must resolve to these receivers
expected_inhibited: true – alert must be muted by an inhibit rule
Running all alerts in a case together allows inhibition to be tested
because the fakeAlertsProvider pre-loads the full case set so the
Inhibitor's initial slurp sees source and target alerts at once.
YAML format:
tests:
- name: "Unmatched alert routes to default"
alerts:
- labels: {alertname: SomeAlert}
expected_receivers: [default]
- name: "critical suppresses warning"
alerts:
- labels: {alertname: SomeAlert, severity: critical}
expected_receivers: [default]
- labels: {alertname: SomeAlert, severity: warning}
expected_inhibited: true
Exit code is 0 when all cases pass, 1 when any fail.
Closes prometheus#5167
Signed-off-by: Ali <alliasgher123@gmail.com>
📝 WalkthroughWalkthroughAdds YAML-driven batch routing test-file support to the CLI command Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as amtool CLI
participant FS as File System
participant Parser as YAML Parser
participant ConfigLoader as Alertmanager Config
participant Inhibitor
participant Router as Route Matcher
participant Results as Test Results
User->>CLI: Run `config routes test --test-file <file>`
CLI->>FS: Read test file
FS-->>CLI: YAML content
CLI->>Parser: Unmarshal test definitions
Parser-->>CLI: Test cases
CLI->>ConfigLoader: Load Alertmanager config
ConfigLoader-->>CLI: Route tree & inhibit rules
loop For each test case
CLI->>CLI: Build deterministic alert objects
alt inhibit rules present
CLI->>Inhibitor: Create with fake alerts provider and Run()
Inhibitor-->>CLI: Ready after processing feed
end
loop For each alert
alt expected_inhibited = true
CLI->>Inhibitor: Mutes(labelset)?
Inhibitor-->>CLI: muted / not muted
CLI->>Results: Record PASS/FAIL
else expected_receivers set
CLI->>Router: Match(labelset)
Router-->>CLI: ordered receivers
CLI->>Results: Compare & record PASS/FAIL
end
end
end
Results-->>CLI: overall pass/fail
CLI->>User: exit 0 if all pass else exit 1
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
cli/test_routing.go (1)
76-88: Reject mixed--test-fileand single-alert inputs.When this branch runs, positional
labels,--verify.receivers, and--treeare silently ignored. That makes bad CI invocations hard to spot; return a usage error instead of discarding those options.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/test_routing.go` around lines 76 - 88, In routingTestAction, when c.testFile != "" add validation to reject mixed usage: if a test file is provided while positional labels are present or flags --verify.receivers or --tree are set, return a usage error instead of silently ignoring them. Check the presence of c.labels (or equivalent positional slice), c.verify.Receivers (or the struct field used), and c.tree, and return a clear error (via kingpin.Fatalf or an error return) indicating that --test-file cannot be combined with positional labels, --verify.receivers, or --tree so callers see the misuse immediately.cli/routes_test_file_test.go (1)
128-174: Add regressions for invalid alert specs and inhibited receiver assertions.The happy-path coverage is good, but the suite does not pin the two edge cases most likely to regress here: invalid YAML entries that set both/neither expectation fields, and alerts that are inhibited while the test asserts
expected_receivers. Once the runner validates those cases explicitly, please add coverage for them here.Also applies to: 191-238
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/routes_test_file_test.go` around lines 128 - 174, Add two regression tests and/or input-validation to catch invalid alert specs and mismatched inhibited/receiver assertions: (1) add a test (e.g., TestRunRouteTestCase_InvalidAlertSpec) using runRouteTestCase with a testFileAlertDef that sets both ExpectedInhibited and non-empty ExpectedReceivers (and another with neither set) and assert the runner returns failure with an error message about invalid expectation fields; (2) add a test (e.g., TestRunRouteTestCase_InhibitedButExpectedReceivers) where an alert is actually inhibited (use inhibitRules from loadTestRoute) but the testFileAlertDef includes ExpectedReceivers, and assert runRouteTestCase fails with a message indicating "inhibited but expected_receivers" (or similar). Also update runRouteTestCase to validate testFileAlertDef (fields ExpectedInhibited and ExpectedReceivers) and return clear failures when both/none are set or when an inhibited alert is asserted to have receivers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/routes_test_file.go`:
- Around line 194-221: The test incorrectly checks inhibition only when
ad.ExpectedInhibited is true, allowing muted alerts to still validate receiver
expectations; change the logic in the alert assertion block so you always check
inhibition first by calling ih.Mutes(context.Background(), lset) (handle ih ==
nil as before) and if muted, treat the alert as inhibited (fail receiver
assertions or compare against ExpectedInhibited), otherwise proceed to call
mainRoute.Match(lset) and validate ad.ExpectedReceivers; update the branch that
uses ad.ExpectedInhibited, ih.Mutes, mainRoute.Match, and equalStringSlices to
ensure receiver assertions are skipped or fail when ih.Mutes reports muted.
- Around line 37-45: Change testFileAlertDef.ExpectedInhibited from bool to
*bool (tri-state) and add validation to ensure exactly one of ExpectedReceivers
(non-empty) vs ExpectedInhibited (non-nil) is provided; implement a Validate()
method on testFileAlertDef that returns an error when (len(ExpectedReceivers) >
0) == (ExpectedInhibited != nil) (covers both both-present and both-absent
cases) and call this Validate() after parsing the YAML and before executing
assertions so invalid specs are rejected.
---
Nitpick comments:
In `@cli/routes_test_file_test.go`:
- Around line 128-174: Add two regression tests and/or input-validation to catch
invalid alert specs and mismatched inhibited/receiver assertions: (1) add a test
(e.g., TestRunRouteTestCase_InvalidAlertSpec) using runRouteTestCase with a
testFileAlertDef that sets both ExpectedInhibited and non-empty
ExpectedReceivers (and another with neither set) and assert the runner returns
failure with an error message about invalid expectation fields; (2) add a test
(e.g., TestRunRouteTestCase_InhibitedButExpectedReceivers) where an alert is
actually inhibited (use inhibitRules from loadTestRoute) but the
testFileAlertDef includes ExpectedReceivers, and assert runRouteTestCase fails
with a message indicating "inhibited but expected_receivers" (or similar). Also
update runRouteTestCase to validate testFileAlertDef (fields ExpectedInhibited
and ExpectedReceivers) and return clear failures when both/none are set or when
an inhibited alert is asserted to have receivers.
In `@cli/test_routing.go`:
- Around line 76-88: In routingTestAction, when c.testFile != "" add validation
to reject mixed usage: if a test file is provided while positional labels are
present or flags --verify.receivers or --tree are set, return a usage error
instead of silently ignoring them. Check the presence of c.labels (or equivalent
positional slice), c.verify.Receivers (or the struct field used), and c.tree,
and return a clear error (via kingpin.Fatalf or an error return) indicating that
--test-file cannot be combined with positional labels, --verify.receivers, or
--tree so callers see the misuse immediately.
🪄 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 Plus
Run ID: eee5c203-4bea-4437-a6c1-b05054e52b69
📒 Files selected for processing (5)
cli/routes_test_file.gocli/routes_test_file_test.gocli/routing.gocli/test_routing.gocli/testdata/conf.inhibit.yml
- Use *bool for ExpectedInhibited to distinguish 'not set' from false; validate that exactly one of expected_receivers/expected_inhibited is set per alert entry - When an inhibitor is active, check mute status before receiver matching so that a receiver assertion on an actually-inhibited alert fails rather than incorrectly passing - Add TestRunRouteTestCase_ReceiverExpectedButMuted to cover the new failure path Signed-off-by: Ali <alliasgher123@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cli/routes_test_file_test.go (1)
211-258: Add invalid/empty spec regression tests after validation hardening.Once runtime validation is added, include explicit tests for
tests: []and for a case withalerts: []so false-green behavior cannot regress.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/routes_test_file_test.go` around lines 211 - 258, Add two regression tests that call executeRoutesTestFile with an empty test spec and with an alert that has an empty alerts list to prevent false-green behavior: create TestExecuteRoutesTestFile_EmptyTests where the YAML contains "tests: []" and assert the call returns an error and/or passed == false (use require.Error and require.False), and create TestExecuteRoutesTestFile_EmptyAlerts where a test entry has "alerts: []" and similarly assert validation fails via require.Error and require.False; reference executeRoutesTestFile and the YAML keys tests: [] and alerts: [] when adding these tests near the existing TestExecuteRoutesTestFile_* functions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/routes_test_file.go`:
- Around line 172-177: The test currently skips assertions when a case defines
no alerts; add an explicit failure when tc.Alerts is empty to avoid false
passes—before the loop that builds allAlerts (where tc.Alerts and buildTestAlert
are used), check if len(tc.Alerts) == 0 and call t.Fatalf or t.Errorf with a
clear message including the test case identifier (e.g., tc.Name or index) so
cases that supply no alerts fail immediately and do not return (true, "")
erroneously.
- Around line 148-159: The current logic sets allPassed := true and if tf.Tests
is empty the function returns success; change this to explicitly reject empty
test files by checking if len(tf.Tests) == 0 before the loop and returning
(false, error) or an appropriate non-success result; alternatively initialize
allPassed := false and only set true if at least one test ran, but preferred is
to add a pre-loop guard that inspects tf.Tests and returns a clear error
(mention tf.Tests, allPassed, runRouteTestCase, mainRoute, inhibitRules) so
empty test collections are treated as failure rather than silent success.
---
Nitpick comments:
In `@cli/routes_test_file_test.go`:
- Around line 211-258: Add two regression tests that call executeRoutesTestFile
with an empty test spec and with an alert that has an empty alerts list to
prevent false-green behavior: create TestExecuteRoutesTestFile_EmptyTests where
the YAML contains "tests: []" and assert the call returns an error and/or passed
== false (use require.Error and require.False), and create
TestExecuteRoutesTestFile_EmptyAlerts where a test entry has "alerts: []" and
similarly assert validation fails via require.Error and require.False; reference
executeRoutesTestFile and the YAML keys tests: [] and alerts: [] when adding
these tests near the existing TestExecuteRoutesTestFile_* functions.
🪄 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 Plus
Run ID: bc309204-c8f6-4529-93b4-c2988c05f900
📒 Files selected for processing (2)
cli/routes_test_file.gocli/routes_test_file_test.go
Comment on lines
+148
to
+159
| allPassed := true | ||
| for _, tc := range tf.Tests { | ||
| passed, detail := runRouteTestCase(tc, mainRoute, inhibitRules) | ||
| if passed { | ||
| fmt.Printf("PASS %s\n", tc.Name) | ||
| } else { | ||
| fmt.Printf("FAIL %s / %s\n", tc.Name, detail) | ||
| allPassed = false | ||
| } | ||
| } | ||
|
|
||
| return allPassed, nil |
There was a problem hiding this comment.
Reject empty tests files instead of returning success.
At Line 148, allPassed defaults to true; if tf.Tests is empty, the loop never runs and the command returns success with zero executed assertions.
Proposed fix
allPassed := true
+ if len(tf.Tests) == 0 {
+ return false, fmt.Errorf("test file %q must contain at least one test case", testFilePath)
+ }
for _, tc := range tf.Tests {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| allPassed := true | |
| for _, tc := range tf.Tests { | |
| passed, detail := runRouteTestCase(tc, mainRoute, inhibitRules) | |
| if passed { | |
| fmt.Printf("PASS %s\n", tc.Name) | |
| } else { | |
| fmt.Printf("FAIL %s / %s\n", tc.Name, detail) | |
| allPassed = false | |
| } | |
| } | |
| return allPassed, nil | |
| allPassed := true | |
| if len(tf.Tests) == 0 { | |
| return false, fmt.Errorf("test file %q must contain at least one test case", testFilePath) | |
| } | |
| for _, tc := range tf.Tests { | |
| passed, detail := runRouteTestCase(tc, mainRoute, inhibitRules) | |
| if passed { | |
| fmt.Printf("PASS %s\n", tc.Name) | |
| } else { | |
| fmt.Printf("FAIL %s / %s\n", tc.Name, detail) | |
| allPassed = false | |
| } | |
| } | |
| return allPassed, nil |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/routes_test_file.go` around lines 148 - 159, The current logic sets
allPassed := true and if tf.Tests is empty the function returns success; change
this to explicitly reject empty test files by checking if len(tf.Tests) == 0
before the loop and returning (false, error) or an appropriate non-success
result; alternatively initialize allPassed := false and only set true if at
least one test ran, but preferred is to add a pre-loop guard that inspects
tf.Tests and returns a clear error (mention tf.Tests, allPassed,
runRouteTestCase, mainRoute, inhibitRules) so empty test collections are treated
as failure rather than silent success.
Comment on lines
+172
to
+177
| // Build types.Alert slice for all alerts in this case so the inhibitor | ||
| // can see the whole set at once. | ||
| allAlerts := make([]*types.Alert, 0, len(tc.Alerts)) | ||
| for _, ad := range tc.Alerts { | ||
| allAlerts = append(allAlerts, buildTestAlert(ad.Labels, now)) | ||
| } |
There was a problem hiding this comment.
Fail test cases that define no alerts.
At Line 172 onward, an empty tc.Alerts executes no assertions and reaches Line 258 returning (true, ""). That creates false PASS cases despite no routed/inhibited behavior being tested.
Proposed fix
func runRouteTestCase(
tc testFileCase,
mainRoute *dispatch.Route,
inhibitRules []amcommoncfg.InhibitRule,
) (bool, string) {
+ if len(tc.Alerts) == 0 {
+ return false, fmt.Sprintf("test case %q must contain at least one alert", tc.Name)
+ }
+
now := time.Now()Also applies to: 258-258
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/routes_test_file.go` around lines 172 - 177, The test currently skips
assertions when a case defines no alerts; add an explicit failure when tc.Alerts
is empty to avoid false passes—before the loop that builds allAlerts (where
tc.Alerts and buildTestAlert are used), check if len(tc.Alerts) == 0 and call
t.Fatalf or t.Errorf with a clear message including the test case identifier
(e.g., tc.Name or index) so cases that supply no alerts fail immediately and do
not return (true, "") erroneously.
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
Extends
amtool config routes testwith a--test-fileflag that accepts a YAML file of named test cases. Each case fires a set of alerts together and asserts per-alert expectations, making it possible to test both routing and inhibition in a repeatable, CI-friendly way.Closes #5167
Usage
Exit code 0 on all pass, 1 on any failure.
Implementation
cli/routes_test_file.go— YAML structs,fakeAlertsProvider(minimalprovider.Alertsthat pre-loads a fixed alert set for inhibition), andexecuteRoutesTestFile/runRouteTestCaserunnercli/test_routing.go— adds--test-fileflag; delegates toexecuteRoutesTestFilewhen setcli/routes_test_file_test.go— 13 new tests (routing pass/fail, multi-receivercontinue: true, inhibition pass/fail, error cases, end-to-end integration)Routing uses the existing
dispatch.Route.Match()path. Inhibition constructs aninhibit.Inhibitorwith afakeAlertsProviderthat returns all alerts in the case viaSubscribe(), callsih.Run()+ih.WaitForLoading()to let the inhibitor process the feed, then checksih.Mutes()per alert.Summary by CodeRabbit
New Features
Tests