-
Notifications
You must be signed in to change notification settings - Fork 85
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
Fix "slice bounds out of range" error in memoryMetric.selectPoints() #30
Conversation
@fedorlitau Thank you for your contribution! Sorry I got late. I'm in the middle of traveling so let me take a look later. Btw, seems like your git commits aren't linked to your Github account (Ali's PR either). Because I want to accept you as a contributor, could you configure that setting if possible? |
98ca6d3
to
13cd440
Compare
@nakabonne Good point - I've corrected the commits on both PR's. Enjoy the traveling! ;-) |
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.
Well noticed! Thank you for working on this problem ;)
@fedorlitau In addition to that, could you add this test case to diff --git a/memory_partition_test.go b/memory_partition_test.go
index 625d0bc..0e0701b 100644
--- a/memory_partition_test.go
+++ b/memory_partition_test.go
@@ -84,7 +84,43 @@ func Test_memoryPartition_SelectDataPoints(t *testing.T) {
want: []*DataPoint{},
},
{
- name: "select multiple points",
+ name: "select some points",
+ metric: "metric1",
+ start: 2,
+ end: 4,
+ memoryPartition: func() *memoryPartition {
+ m := newMemoryPartition(nil, 0, "").(*memoryPartition)
+ m.insertRows([]Row{
+ {
+ Metric: "metric1",
+ DataPoint: DataPoint{Timestamp: 1, Value: 0.1},
+ },
+ {
+ Metric: "metric1",
+ DataPoint: DataPoint{Timestamp: 2, Value: 0.1},
+ },
+ {
+ Metric: "metric1",
+ DataPoint: DataPoint{Timestamp: 3, Value: 0.1},
+ },
+ {
+ Metric: "metric1",
+ DataPoint: DataPoint{Timestamp: 4, Value: 0.1},
+ },
+ {
+ Metric: "metric1",
+ DataPoint: DataPoint{Timestamp: 5, Value: 0.1},
+ },
+ })
+ return m
+ }(),
+ want: []*DataPoint{
+ {Timestamp: 2, Value: 0.1},
+ {Timestamp: 3, Value: 0.1},
+ },
+ },
+ {
+ name: "select all points",
metric: "metric1",
start: 1,
end: 4, |
13cd440
to
b92d1c9
Compare
Codecov Report
@@ Coverage Diff @@
## main #30 +/- ##
==========================================
+ Coverage 65.35% 66.08% +0.74%
==========================================
Files 19 19
Lines 955 955
==========================================
+ Hits 624 631 +7
+ Misses 234 229 -5
+ Partials 97 95 -2
Continue to review full report at Codecov.
|
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.
Great work!
Thank you! And thank you for incorporating the fixes quicky and for: |
This will fix a bug which I have encountered while using nakabonne/ali (awesome! btw):
In the original code almost always the first case (green) is entered - the bug occurs when the second (red) case is executed: