- fix after review: minir refactoring + added more unit tests

multiversx · Aug 31, 2022 · 81024e1 · 81024e1
1 parent 809d9c7
commit 81024e1
Show file tree

Hide file tree

Showing 4 changed files with 76 additions and 27 deletions.
diff --git a/storage/timecache/timeCacheCore.go b/storage/timecache/timeCacheCore.go
@@ -58,7 +58,7 @@ func (tcc *timeCacheCore) upsert(key string, value interface{}, duration time.Du
 }
 
 // put will add the key, value and provided duration, overriding values if the data already existed
-// It returns if the value existed before this call. It also operates on the locker so the call is concurrent safe
+// It returns true if the value existed before this call. It also operates on the locker so the call is concurrent safe
 func (tcc *timeCacheCore) put(key string, value interface{}, duration time.Duration) (bool, error) {
 	if len(key) == 0 {
 		return false, storage.ErrEmptyKey
@@ -78,26 +78,26 @@ func (tcc *timeCacheCore) put(key string, value interface{}, duration time.Durat
 }
 
 // hasOrAdd will add the key, value and provided duration, if the key is not found
-// It returns if the value existed before this call and if it has been added or not. It also operates on the locker so the call is concurrent safe
-func (tcc *timeCacheCore) hasOrAdd(key string, value interface{}, duration time.Duration) (bool, bool) {
+// It returns true if the value existed before this call and if it has been added or not. It also operates on the locker so the call is concurrent safe
+func (tcc *timeCacheCore) hasOrAdd(key string, value interface{}, duration time.Duration) (bool, bool, error) {
 	if len(key) == 0 {
-		return false, false
+		return false, false, storage.ErrEmptyKey
 	}
 
 	tcc.Lock()
 	defer tcc.Unlock()
 
 	_, found := tcc.data[key]
 	if found {
-		return found, false
+		return found, false, nil
 	}
 
 	tcc.data[key] = &entry{
 		timestamp: time.Now(),
 		span:      duration,
 		value:     value,
 	}
-	return false, true
+	return false, true, nil
 }
 
 // sweep iterates over all contained elements checking if the element is still valid to be kept

diff --git a/storage/timecache/timeCacheCore_test.go b/storage/timecache/timeCacheCore_test.go
@@ -36,7 +36,8 @@ func TestTimeCacheCore_ConcurrentOperations(t *testing.T) {
 				_, err := tcc.put(fmt.Sprintf("key%d", idx), fmt.Sprintf("valuey%d", idx), time.Second)
 				assert.Nil(t, err)
 			case 6:
-				_, _ = tcc.hasOrAdd(fmt.Sprintf("key%d", idx), fmt.Sprintf("valuey%d", idx), time.Second)
+				_, _, err := tcc.hasOrAdd(fmt.Sprintf("key%d", idx), fmt.Sprintf("valuey%d", idx), time.Second)
+				assert.Nil(t, err)
 			default:
 				assert.Fail(t, "test setup error, change the line 'switch idx % xxx {' from this test")
 			}

diff --git a/storage/timecache/timeCacher.go b/storage/timecache/timeCacher.go
@@ -115,7 +115,15 @@ func (tc *timeCacher) Peek(key []byte) (value interface{}, ok bool) {
 // HasOrAdd checks if a key is in the cache.
 // If key exists, does not update the value. Otherwise, adds the key-value in the cache
 func (tc *timeCacher) HasOrAdd(key []byte, value interface{}, _ int) (has, added bool) {
-	return tc.timeCache.hasOrAdd(string(key), value, tc.timeCache.defaultSpan)
+	var err error
+	has, added, err = tc.timeCache.hasOrAdd(string(key), value, tc.timeCache.defaultSpan)
+	if err != nil {
+		log.Error("mapTimeCacher.HasOrAdd", "error", key)
+
+		return
+	}
+
+	return
 }
 
 // Remove removes the key from cache

diff --git a/storage/timecache/timeCacher_test.go b/storage/timecache/timeCacher_test.go
@@ -123,17 +123,38 @@ func TestTimeCacher_Has(t *testing.T) {
 func TestTimeCacher_HasOrAdd(t *testing.T) {
 	t.Parallel()
 
-	cacher, _ := timecache.NewTimeCacher(createArgTimeCacher())
-	assert.False(t, cacher.IsInterfaceNil())
+	t.Run("empty or nil key should return false, false", func(t *testing.T) {
+		t.Parallel()
 
-	providedKey, providedVal := []byte("key"), []byte("val")
-	has, added := cacher.HasOrAdd(providedKey, providedVal, len(providedVal))
-	assert.False(t, has)
-	assert.True(t, added)
+		cacher, _ := timecache.NewTimeCacher(createArgTimeCacher())
+		t.Run("nil key", func(t *testing.T) {
+			has, added := cacher.HasOrAdd(nil, nil, 0)
+			assert.False(t, has)
+			assert.False(t, added)
+			assert.Equal(t, 0, cacher.Len())
+		})
+		t.Run("empty key", func(t *testing.T) {
+			has, added := cacher.HasOrAdd(make([]byte, 0), nil, 0)
+			assert.False(t, has)
+			assert.False(t, added)
+			assert.Equal(t, 0, cacher.Len())
+		})
+	})
+	t.Run("should work", func(t *testing.T) {
+		t.Parallel()
+
+		cacher, _ := timecache.NewTimeCacher(createArgTimeCacher())
+		assert.False(t, cacher.IsInterfaceNil())
 
-	has, added = cacher.HasOrAdd(providedKey, providedVal, len(providedVal))
-	assert.True(t, has)
-	assert.False(t, added)
+		providedKey, providedVal := []byte("key"), []byte("val")
+		has, added := cacher.HasOrAdd(providedKey, providedVal, len(providedVal))
+		assert.False(t, has)
+		assert.True(t, added)
+
+		has, added = cacher.HasOrAdd(providedKey, providedVal, len(providedVal))
+		assert.True(t, has)
+		assert.False(t, added)
+	})
 }
 
 func TestTimeCacher_Keys(t *testing.T) {
@@ -203,17 +224,36 @@ func TestTimeCacher_Peek(t *testing.T) {
 func TestTimeCacher_Put(t *testing.T) {
 	t.Parallel()
 
-	cacher, _ := timecache.NewTimeCacher(createArgTimeCacher())
-	assert.False(t, cacher.IsInterfaceNil())
+	t.Run("empty or nil key should return false, false", func(t *testing.T) {
+		t.Parallel()
 
-	numOfPairs := 2
-	keys, vals := createKeysVals(numOfPairs)
-	evicted := cacher.Put(keys[0], vals[0], len(vals[0]))
-	assert.False(t, evicted)
-	assert.Equal(t, 1, cacher.Len())
-	evicted = cacher.Put(keys[0], vals[1], len(vals[1]))
-	assert.False(t, evicted)
-	assert.Equal(t, 1, cacher.Len())
+		cacher, _ := timecache.NewTimeCacher(createArgTimeCacher())
+		t.Run("nil key", func(t *testing.T) {
+			evicted := cacher.Put(nil, nil, 0)
+			assert.False(t, evicted)
+			assert.Equal(t, 0, cacher.Len())
+		})
+		t.Run("empty key", func(t *testing.T) {
+			evicted := cacher.Put(make([]byte, 0), nil, 0)
+			assert.False(t, evicted)
+			assert.Equal(t, 0, cacher.Len())
+		})
+	})
+	t.Run("should work", func(t *testing.T) {
+		t.Parallel()
+
+		cacher, _ := timecache.NewTimeCacher(createArgTimeCacher())
+		assert.False(t, cacher.IsInterfaceNil())
+
+		numOfPairs := 2
+		keys, vals := createKeysVals(numOfPairs)
+		evicted := cacher.Put(keys[0], vals[0], len(vals[0]))
+		assert.False(t, evicted)
+		assert.Equal(t, 1, cacher.Len())
+		evicted = cacher.Put(keys[0], vals[1], len(vals[1]))
+		assert.False(t, evicted)
+		assert.Equal(t, 1, cacher.Len())
+	})
 }
 
 func TestTimeCacher_Remove(t *testing.T) {