From cff567a750aa6dff5243d1be89e619592ab2fecd Mon Sep 17 00:00:00 2001 From: Noble Mittal Date: Mon, 29 Jan 2024 21:37:44 +0530 Subject: [PATCH 1/3] Add missing tests for go/vt/vtorc/collection Signed-off-by: Noble Mittal --- go/vt/vtorc/collection/collection.go | 7 +- go/vt/vtorc/collection/collection_test.go | 115 ++++++++++++++++++++++ 2 files changed, 120 insertions(+), 2 deletions(-) diff --git a/go/vt/vtorc/collection/collection.go b/go/vt/vtorc/collection/collection.go index 0ef9a71b9a3..83681076af9 100644 --- a/go/vt/vtorc/collection/collection.go +++ b/go/vt/vtorc/collection/collection.go @@ -61,6 +61,7 @@ package collection import ( "errors" + "fmt" "sync" "time" @@ -146,6 +147,8 @@ func (c *Collection) StopAutoExpiration() { return } c.monitoring = false + fmt.Print(c.monitoring) + c.Unlock() // no locking here deliberately @@ -260,8 +263,8 @@ func (c *Collection) removeBefore(t time.Time) error { // get the interval we need. if first == len(c.collection) { c.collection = nil // remove all entries - } else if first != -1 { - c.collection = c.collection[first:] + } else { + c.collection = c.collection[first+1:] } return nil // no errors } diff --git a/go/vt/vtorc/collection/collection_test.go b/go/vt/vtorc/collection/collection_test.go index 23679245c26..025e7728409 100644 --- a/go/vt/vtorc/collection/collection_test.go +++ b/go/vt/vtorc/collection/collection_test.go @@ -19,6 +19,8 @@ package collection import ( "testing" "time" + + "github.com/stretchr/testify/assert" ) var randomString = []string{ @@ -28,6 +30,7 @@ var randomString = []string{ // some random base timestamp var ts = time.Date(2016, 12, 27, 13, 36, 40, 0, time.Local) +var ts2 = ts.AddDate(-1, 0, 0) // TestCreateOrReturn tests the creation of a named Collection func TestCreateOrReturnCollection(t *testing.T) { @@ -87,6 +90,13 @@ func (tm *testMetric) When() time.Time { return ts } +type testMetric2 struct { +} + +func (tm *testMetric2) When() time.Time { + return ts2 +} + // check that Append() works as expected func TestAppend(t *testing.T) { c := &Collection{} @@ -101,4 +111,109 @@ func TestAppend(t *testing.T) { t.Errorf("TestExpirePeriod: len(Metrics) = %d, expecting %d", len(c.Metrics()), v) } } + + // Test for nil metric + err := c.Append(nil) + assert.Error(t, err) + assert.Equal(t, err.Error(), "Collection.Append: m == nil") +} + +func TestNilCollection(t *testing.T) { + var c *Collection + + metrics := c.Metrics() + assert.Nil(t, metrics) + + err := c.Append(nil) + assert.Error(t, err) + assert.Equal(t, err.Error(), "Collection.Append: c == nil") + + err = c.removeBefore(ts) + assert.Error(t, err) + assert.Equal(t, err.Error(), "Collection.removeBefore: c == nil") + + // Should not throw any error for nil Collection + c.StartAutoExpiration() + c.StopAutoExpiration() +} + +func TestStopAutoExpiration(t *testing.T) { + // Clear Collection map + namedCollection = make(map[string]*Collection) + + name := randomString[0] + c := CreateOrReturnCollection(name) + + c.StopAutoExpiration() + assert.False(t, c.monitoring) + + // Test when c.monitoring == true before calling StartAutoExpiration + c.monitoring = true + c.StartAutoExpiration() + assert.True(t, c.monitoring) +} + +func TestSince(t *testing.T) { + // Clear Collection map + namedCollection = make(map[string]*Collection) + + name := randomString[0] + + var c *Collection + metrics, err := c.Since(ts) + + assert.Nil(t, metrics) + assert.Error(t, err) + assert.Equal(t, err.Error(), "Collection.Since: c == nil") + + c = CreateOrReturnCollection(name) + metrics, err = c.Since(ts) + assert.Nil(t, metrics) + assert.Nil(t, err) + + tm := &testMetric{} + tm2 := &testMetric2{} + _ = c.Append(tm2) + _ = c.Append(tm) + + metrics, err = c.Since(ts2) + assert.Equal(t, []Metric{tm2, tm}, metrics) + assert.Nil(t, err) + + metrics, err = c.Since(ts) + assert.Equal(t, []Metric{tm}, metrics) + assert.Nil(t, err) +} + +func TestRemoveBefore(t *testing.T) { + // Clear Collection map + namedCollection = make(map[string]*Collection) + + name := randomString[0] + c := CreateOrReturnCollection(name) + + tm := &testMetric{} + tm2 := &testMetric2{} + + err := c.Append(tm2) + assert.Nil(t, err) + + err = c.Append(tm) + assert.Nil(t, err) + + err = c.removeBefore(ts) + assert.NoError(t, err) + assert.Equal(t, []Metric{tm}, c.collection) + + ts3 := ts.AddDate(1, 0, 0) + err = c.removeBefore(ts3) + assert.NoError(t, err) + assert.Nil(t, c.collection) + + name = randomString[1] + c = CreateOrReturnCollection(name) + + err = c.removeBefore(ts) + assert.NoError(t, err) + assert.Equal(t, []Metric(nil), c.collection) } From 806728d0b2d9e1c6d6690cea90c9935b75715e5e Mon Sep 17 00:00:00 2001 From: Noble Mittal Date: Mon, 29 Jan 2024 22:00:47 +0530 Subject: [PATCH 2/3] Remove debug statement from collection.go Signed-off-by: Noble Mittal --- go/vt/vtorc/collection/collection.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/go/vt/vtorc/collection/collection.go b/go/vt/vtorc/collection/collection.go index 83681076af9..4f679286978 100644 --- a/go/vt/vtorc/collection/collection.go +++ b/go/vt/vtorc/collection/collection.go @@ -61,7 +61,6 @@ package collection import ( "errors" - "fmt" "sync" "time" @@ -147,8 +146,6 @@ func (c *Collection) StopAutoExpiration() { return } c.monitoring = false - fmt.Print(c.monitoring) - c.Unlock() // no locking here deliberately From 8e3ae94a133e2e26f583668799628eadbcf1c3cf Mon Sep 17 00:00:00 2001 From: Noble Mittal Date: Mon, 5 Feb 2024 02:15:58 +0530 Subject: [PATCH 3/3] Restore value of the global variables Signed-off-by: Noble Mittal --- go/vt/vtorc/collection/collection_test.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/go/vt/vtorc/collection/collection_test.go b/go/vt/vtorc/collection/collection_test.go index 025e7728409..16ea6943dc7 100644 --- a/go/vt/vtorc/collection/collection_test.go +++ b/go/vt/vtorc/collection/collection_test.go @@ -138,6 +138,10 @@ func TestNilCollection(t *testing.T) { } func TestStopAutoExpiration(t *testing.T) { + oldNamedCollection := namedCollection + defer func() { + namedCollection = oldNamedCollection + }() // Clear Collection map namedCollection = make(map[string]*Collection) @@ -154,6 +158,10 @@ func TestStopAutoExpiration(t *testing.T) { } func TestSince(t *testing.T) { + oldNamedCollection := namedCollection + defer func() { + namedCollection = oldNamedCollection + }() // Clear Collection map namedCollection = make(map[string]*Collection) @@ -186,6 +194,10 @@ func TestSince(t *testing.T) { } func TestRemoveBefore(t *testing.T) { + oldNamedCollection := namedCollection + defer func() { + namedCollection = oldNamedCollection + }() // Clear Collection map namedCollection = make(map[string]*Collection)