Fix silence tests #8283
Fix silence tests #8283
Conversation
grobinson-grafana
force-pushed
the
grobinson/fix-silence-tests
branch
5 times, most recently
from
271b117 to
27ecae9
Compare
| // Set the interval to 1s as this test can trigger multiple broadcasts | ||
| // creating and expiring silences. | ||
| PersisterConfig: PersisterConfig{Interval: time.Second}, | ||
| // We have set this 1 minute, but we don't use it in this |
There was a problem hiding this comment.
If we don't use the interval, why do we have to change the current value? Or even better, why do we have to set it?
There was a problem hiding this comment.
Good question. Most tests use 1 hour for the interval, and this test used to use 1 hour before I changed it to 1 second, thinking the interval was the issue. However, I found that the interval is not the issue, so I increased it from 1 second to 1 minute. I can also change it back to 1 hour which will be a no-op – but at least consistent with the other tests.
There was a problem hiding this comment.
Is it possible not to specify the interval length at all, since it is not used? If it is not possible, then I would set it to 1h in order to be consistent with other tests, and I would write a comment stating that we set it to 1h for consistency, but that actually it will be overridden by a call to Setxxx. WDYT?
There was a problem hiding this comment.
It's not possible I'm afraid, otherwise it panics with panic: non-positive interval for NewTicker. I've set it to 1h 👍
There is an existing comment:
// We have set this to 1 hour, but we don't use it in this test as we override the broadcast function with SetBroadcast.
Is that sufficient?
| @@ -373,8 +374,13 @@ func TestSilenceLimits(t *testing.T) { | |||
| require.EqualError(t, err, "exceeded maximum number of silences: 1 (limit: 1)") | |||
| require.Equal(t, "", id2) | |||
|
|
|||
| // Expire sil1. This should allow sil2 to be inserted. | |||
| // Expire sil1 and run the GC. This should allow sil2 to be | |||
There was a problem hiding this comment.
From the comment it looks like you run the GC on sil1 only, but you actually run it on both sil1 and sil2.
There was a problem hiding this comment.
sil2 hasn't been set at this time, so the GC just affects sil1. sil2 is no inserted until Line 384.
This commit fixes the silence tests, using the exact same tests in prometheus/alertmanager to avoid regressions due to modules, but using the New function in pkg/alertmanager/alertmanager.go to ensure the correct limits are set.
grobinson-grafana
force-pushed
the
grobinson/fix-silence-tests
branch
from
27ecae9 to
c83cb28
Compare
What this PR does
This commit fixes the silence tests, using the exact same tests in prometheus/alertmanager to avoid regressions due to modules, but using the
Newfunction inpkg/alertmanager/alertmanager.goto ensure the correct limits are set.It bumpsgo.modto commitdb27a98which is a no-op as Mimir does not vendor tests, but its meant to indicate we are using the fix from prometheus/alertmanager#3866.The changes to
go.modare no longer needed as prometheus/alertmanager#3866 is included in #8200.Which issue(s) this PR fixes or relates to
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.