-
Notifications
You must be signed in to change notification settings - Fork 1
Conversation
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
c474223
to
6b859af
Compare
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
working_directory: ~/plugin | ||
steps: | ||
- checkout | ||
- restore_cache: | ||
keys: | ||
- yarn-packages-{{ checksum "yarn.lock" }} | ||
- run: | ||
name: Install yarn and Go | ||
name: Install node and yarn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICT, the step doesn't install Go.
@@ -53,15 +53,15 @@ jobs: | |||
- ci | |||
build_backend: | |||
docker: | |||
- image: circleci/golang:1.12 | |||
- image: circleci/golang:1.13 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it's better to upgrade to latest Go?
@@ -65,7 +65,7 @@ func parseRule(rule string) (time.Duration, error) { | |||
func (s Series) Resample(rule string, downsampler string, upsampler string, tr grafana.TimeRange) (Series, error) { | |||
interval, err := parseRule(rule) | |||
if err != nil { | |||
return s, fmt.Errorf(`failed to parse "rule" field "%v": %v`, rule, err) | |||
return s, fmt.Errorf(`failed to parse "rule" field %q: %w`, rule, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to wrap the originating error than to just include the message.
@@ -26,7 +26,7 @@ func TestResampleSeries(t *testing.T) { | |||
upsampler: "fillna", | |||
timeRange: grafana.TimeRange{ | |||
From: time.Unix(0, 0), | |||
To: time.Unix(11, 0), | |||
To: time.Unix(4, 0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This triggers the error condition the test case is for. I need feedback though from someone with more domain knowledge on whether it's correct or not. @kylebrandt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fix seems correct. I wonder how we hadn't caught this before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @papagian.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, yay tests!
Change CircleCI configuration to also test back-end. Fixes #2.
I had to modify a test case that would fail, could use feedback on whether my change is correct.