Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support extended durations in promtool unit tests (Fixes #6285) #6297

Open
wants to merge 2 commits into
base: master
from

Conversation

@neufeldtech
Copy link

neufeldtech commented Nov 9, 2019

Fixes #6285

This PR updates the promtool unit testing framework to parse the yaml files with model.Duration instead of time.Duration. This allows the code to support durations such as 1d or 1w and brings this implementation closer to what a user would expect as it matches the supported durations of other prometheus config files.

I'm open to feedback on if there is a more go idiomatic way of performing this fix, rather then doing time.Duration(model.Duration) in several places in the code as I've done here.

…6285)

Signed-off-by: Jordan Neufeld <jordan@neufeldtech.com>
@neufeldtech neufeldtech force-pushed the neufeldtech:6285 branch from fc68679 to 31e2392 Nov 9, 2019
@@ -405,13 +407,13 @@ func orderedGroups(groupsMap map[string]*rules.Group, groupOrderMap map[string]i
func (tg *testGroup) maxEvalTime() time.Duration {
var maxd time.Duration
for _, alert := range tg.AlertRuleTests {
if alert.EvalTime > maxd {
maxd = alert.EvalTime
if time.Duration(alert.EvalTime) > maxd {

This comment has been minimized.

Copy link
@brian-brazil

brian-brazil Nov 9, 2019

Member

Change the type of maxd, so you don't need to convert

@@ -370,7 +372,7 @@ Outer:
// seriesLoadingString returns the input series in PromQL notation.
func (tg *testGroup) seriesLoadingString() string {
result := ""
result += "load " + shortDuration(tg.Interval) + "\n"
result += "load " + shortDuration(time.Duration(tg.Interval)) + "\n"

This comment has been minimized.

Copy link
@brian-brazil

brian-brazil Nov 9, 2019

Member

Hmm, can we simplify here?

Signed-off-by: Jordan Neufeld <jordan@neufeldtech.com>
@neufeldtech neufeldtech force-pushed the neufeldtech:6285 branch from 938ba7d to e7072ac Nov 10, 2019
@neufeldtech neufeldtech requested a review from brian-brazil Nov 10, 2019
@@ -0,0 +1,42 @@
// Copyright 2018 The Prometheus Authors

This comment has been minimized.

Copy link
@codesome

codesome Nov 11, 2019

Member

There is already a PR for unit tests for unit test #6062, I would like to avoid duplication of effort. Can you add these test cases to that PR instead?

This comment has been minimized.

Copy link
@codesome

codesome Nov 11, 2019

Member

Or more like it would be better if we merge that PR first and merge this with the additional test case.

This comment has been minimized.

Copy link
@neufeldtech

neufeldtech Nov 11, 2019

Author

I'm fine with this PR going after #6062 is merged. I had created the one basic test to prove that deserialization of durations such as 1d was now working

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.