Fix promtool alert tests for no alert validation #8462
Conversation
The documentation of `promtool rules test` states "If you want to test if an alerting rule should not be firing, then .. leave 'exp_alerts' empty." This was not working though, as the code always based its checks on the expected alerts in the test definition and didn't look at the alerts that effectively fired. With an empty list for the expected alert, no validation occured! With this change, the code now looks at the alerts that fired and checks this is corresponding to the expected alerts found in the test definition. Signed-off-by: Raphael Bauduin <raphael.bauduin@tessares.net>
|
@simonpasquier and @dgl, do you have an opinion on this? |
|
The existing behaviour is correct, there's no requirement that a unit test tests all the alerts in a rule file, so the tests have to name each alert that is expected to be tested by name. This change would make it impossible to split testing of some alerts out to a different file. (It also just doesn't handle two different alert names firing -- see how it discards the alertname: https://github.com/prometheus/prometheus/pull/8462/files#diff-7effa22958bf71b1531230dc8acc6322aa3a54b332dd85b80211e590b9ae93dfR282) |
|
I added some more tests in #8490 which maybe make it clearer, if I apply this change on top of those tests I get: i.e. it shows how this change wouldn't handle different alertnames. |
|
Thanks for having taken a look and the explanation. The problem I encountered, as posted on the forum, is that I didn't specify the |
|
@rbauduin yes, definitely. |
|
Closing this as the issue is now addressed in #8504. |
The documentation of
promtool rules teststates "If you want to testif an alerting rule should not be firing, then .. leave 'exp_alerts' empty."
This was not working though, as the code always based its checks on the
expected alerts in the test definition and didn't look at the alerts
that effectively fired. With an empty list for the expected alert, no
validation occured!
With this change, the code now looks at the alerts that fired and checks
this is corresponding to the expected alerts found in the test
definition.