Enhance error handling and add error tests in graph package (#127)

* Enhance error handling and add error tests in graph package This commit improves error handling throughout the graph package by: 1. Adding detailed error wrapping using fmt.Errorf with %w verb in the Cache function to provide better error context and maintain error chain 2. Adding comprehensive error injection capabilities to MockStorage by: - Adding error fields for each operation - Implementing error checks in all mock methods - Improving error handling in mock methods 3. Adding TestCacheErrors test suite that verifies error handling for: - ToBeCached errors - GetAllKeys errors - GetNodes errors - SaveCaches errors - ClearCacheStack errors 4. Improving MockStorage implementation: - Better initialization of internal maps - More consistent error handling patterns - Cleaner key construction for custom data - Added existence checks for data retrieval These changes improve debuggability by providing more context in error messages and increase test coverage around error conditions. Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com> * Ignoring the mockgraph coverage Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com> --------- Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
bitbomdev · Nov 12, 2024 · dd3d7f6 · dd3d7f6
1 parent 4c6c3d1
commit dd3d7f6
Show file tree

Hide file tree

Showing 4 changed files with 160 additions and 26 deletions.
diff --git a/codecov.yml b/codecov.yml
@@ -11,4 +11,5 @@ coverage:
 
   ignore:
     - "cmd/**/*"
-    - "gen/**/*"
+    - "gen/**/*" # generated code
+    - "pkg/graph/mockGraph.go" # mockGraph is not covered
diff --git a/pkg/graph/cache.go b/pkg/graph/cache.go
@@ -10,7 +10,7 @@ import (
 func Cache(storage Storage) error {
 	uncachedNodes, err := storage.ToBeCached()
 	if err != nil {
-		return err
+		return fmt.Errorf("error getting uncached nodes: %w", err)
 	}
 	if len(uncachedNodes) == 0 {
 		return nil
@@ -30,17 +30,17 @@ func Cache(storage Storage) error {
 
 	cachedChildren, err := buildCache(uncachedNodes, ChildrenDirection, scc, allNodes)
 	if err != nil {
-		return err
+		return fmt.Errorf("error building cached children: %w", err)
 	}
 
 	cachedParents, err := buildCache(uncachedNodes, ParentsDirection, scc, allNodes)
 	if err != nil {
-		return err
+		return fmt.Errorf("error building cached parents: %w", err)
 	}
 
 	cachedChildKeys, cachedChildValues, err := cachedChildren.GetAllKeysAndValues()
 	if err != nil {
-		return err
+		return fmt.Errorf("error getting cached child keys and values: %w", err)
 	}
 
 	var caches []*NodeCache
@@ -49,7 +49,7 @@ func Cache(storage Storage) error {
 		childId := cachedChildKeys[i]
 		childIntId, err := strconv.Atoi(childId)
 		if err != nil {
-			return err
+			return fmt.Errorf("error converting child key %s to int: %w", childId, err)
 		}
 
 		childBindValue := cachedChildValues[i].Clone()
@@ -65,7 +65,7 @@ func Cache(storage Storage) error {
 	}
 
 	if err := storage.SaveCaches(caches); err != nil {
-		return err
+		return fmt.Errorf("error saving caches: %w", err)
 	}
 	return storage.ClearCacheStack()
 }
@@ -136,7 +136,7 @@ func buildCache(uncachedNodes []uint32, direction Direction, scc map[uint32]uint
 
 	err := addCyclesToBindMap(scc, cache, children, parents, allNodes)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("error adding cycles to bind map: %w", err)
 	}
 
 	nodesToCache := roaring.New()

diff --git a/pkg/graph/cache_test.go b/pkg/graph/cache_test.go
@@ -374,3 +374,64 @@ func TestSimpleCircle(t *testing.T) {
 		assert.Equal(t, dependenciesNoCache.ToArray(), dependencies.ToArray(), "Cached and non-cached dependencies should match")
 	}
 }
+
+// TestCacheErrors tests the Cache function for various error conditions.
+func TestCacheErrors(t *testing.T) {
+	tests := []struct {
+		name      string
+		setupMock func(*MockStorage)
+		wantErr   string
+	}{
+		{
+			name:      "ToBeCached error",
+			setupMock: func(m *MockStorage) { m.ToBeCachedErr = fmt.Errorf("ToBeCached error") },
+			wantErr:   "error getting uncached nodes: ToBeCached error",
+		},
+		{
+			name:      "GetAllKeys error",
+			setupMock: func(m *MockStorage) { m.GetAllKeysErr = fmt.Errorf("GetAllKeys error") },
+			wantErr:   "error getting keys: GetAllKeys error",
+		},
+		{
+			name:      "GetNodes error",
+			setupMock: func(m *MockStorage) { m.GetNodesErr = fmt.Errorf("GetNodes error") },
+			wantErr:   "error getting all nodes: GetNodes error",
+		},
+		{
+			name:      "SaveCaches error",
+			setupMock: func(m *MockStorage) { m.SaveCachesErr = fmt.Errorf("SaveCaches error") },
+			wantErr:   "error saving caches: SaveCaches error",
+		},
+		{
+			name:      "ClearCacheStack error",
+			setupMock: func(m *MockStorage) { m.ClearCacheStackErr = fmt.Errorf("ClearCacheStack error") },
+			wantErr:   "ClearCacheStack error",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			mockStorage := NewMockStorage()
+			nodes := make([]*Node, 3)
+
+			// Create nodes
+			for i := 0; i < 3; i++ {
+				var err error
+				nodes[i], err = AddNode(mockStorage, fmt.Sprintf("type %d", i+1), fmt.Sprintf("metadata %d", i), fmt.Sprintf("name %d", i+1))
+				assert.NoError(t, err)
+			}
+
+			// Create circle: node0 -> node1 -> node2 -> node0
+			for i := 0; i < 3; i++ {
+				err := nodes[i].SetDependency(mockStorage, nodes[(i+1)%3])
+				assert.NoError(t, err)
+			}
+
+			tt.setupMock(mockStorage)
+			err := Cache(mockStorage)
+			if err == nil || err.Error() != tt.wantErr {
+				t.Errorf("Expected error %q, got %v", tt.wantErr, err)
+			}
+		})
+	}
+}
diff --git a/pkg/graph/mockGraph.go b/pkg/graph/mockGraph.go
@@ -16,10 +16,29 @@ type MockStorage struct {
 	nameToID     map[string]uint32
 	cache        map[uint32]*NodeCache
 	toBeCached   []uint32
-	db           map[string]map[string][]byte
 	mu           sync.Mutex
 	idCounter    uint32
 	fullyCached  bool
+	db           map[string]map[string][]byte
+
+	// Error injection fields
+	SaveNodeErr              error
+	GetNodeErr               error
+	GetNodesByGlobErr        error
+	GetAllKeysErr            error
+	SaveCacheErr             error
+	ToBeCachedErr            error
+	AddNodeToCachedStackErr  error
+	ClearCacheStackErr       error
+	GetCacheErr              error
+	GenerateIDErr            error
+	NameToIDErr              error
+	GetNodesErr              error
+	SaveCachesErr            error
+	GetCachesErr             error
+	RemoveAllCachesErr       error
+	AddOrUpdateCustomDataErr error
+	GetCustomDataErr         error
 }
 
 func NewMockStorage() *MockStorage {
@@ -29,10 +48,14 @@ func NewMockStorage() *MockStorage {
 		dependents:   make(map[uint32]*roaring.Bitmap),
 		nameToID:     make(map[string]uint32),
 		idCounter:    0,
+		db:           make(map[string]map[string][]byte),
 	}
 }
 
 func (m *MockStorage) SaveNode(node *Node) error {
+	if m.SaveNodeErr != nil {
+		return m.SaveNodeErr
+	}
 	m.mu.Lock()
 	defer m.mu.Unlock()
 	m.nameToID[node.Name] = node.ID
@@ -42,6 +65,9 @@ func (m *MockStorage) SaveNode(node *Node) error {
 }
 
 func (m *MockStorage) GetNode(id uint32) (*Node, error) {
+	if m.GetNodeErr != nil {
+		return nil, m.GetNodeErr
+	}
 	m.mu.Lock()
 	defer m.mu.Unlock()
 	node, exists := m.nodes[id]
@@ -52,6 +78,9 @@ func (m *MockStorage) GetNode(id uint32) (*Node, error) {
 }
 
 func (m *MockStorage) GetNodesByGlob(pattern string) ([]*Node, error) {
+	if m.GetNodesByGlobErr != nil {
+		return nil, m.GetNodesByGlobErr
+	}
 	m.mu.Lock()
 	defer m.mu.Unlock()
 
@@ -69,6 +98,9 @@ func (m *MockStorage) GetNodesByGlob(pattern string) ([]*Node, error) {
 }
 
 func (m *MockStorage) GetAllKeys() ([]uint32, error) {
+	if m.GetAllKeysErr != nil {
+		return nil, m.GetAllKeysErr
+	}
 	m.mu.Lock()
 	defer m.mu.Unlock()
 
@@ -81,6 +113,9 @@ func (m *MockStorage) GetAllKeys() ([]uint32, error) {
 }
 
 func (m *MockStorage) SaveCache(cache *NodeCache) error {
+	if m.SaveCacheErr != nil {
+		return m.SaveCacheErr
+	}
 	m.mu.Lock()
 	defer m.mu.Unlock()
 	if m.cache == nil {
@@ -91,49 +126,68 @@ func (m *MockStorage) SaveCache(cache *NodeCache) error {
 }
 
 func (m *MockStorage) ToBeCached() ([]uint32, error) {
+	if m.ToBeCachedErr != nil {
+		return nil, m.ToBeCachedErr
+	}
 	m.mu.Lock()
 	defer m.mu.Unlock()
 	return m.toBeCached, nil
 }
 
 func (m *MockStorage) AddNodeToCachedStack(id uint32) error {
+	if m.AddNodeToCachedStackErr != nil {
+		return m.AddNodeToCachedStackErr
+	}
 	m.toBeCached = append(m.toBeCached, id)
-
 	return nil
 }
 
 func (m *MockStorage) ClearCacheStack() error {
+	if m.ClearCacheStackErr != nil {
+		return m.ClearCacheStackErr
+	}
 	m.toBeCached = []uint32{}
-
 	return nil
 }
 
 func (m *MockStorage) GetCache(id uint32) (*NodeCache, error) {
+	if m.GetCacheErr != nil {
+		return nil, m.GetCacheErr
+	}
 	m.mu.Lock()
 	defer m.mu.Unlock()
 	if _, ok := m.cache[id]; !ok {
-		return nil, errors.New("cacheHelper not found")
+		return nil, errors.New("cache not found")
 	}
 	return m.cache[id], nil
 }
 
 func (m *MockStorage) GenerateID() (uint32, error) {
+	if m.GenerateIDErr != nil {
+		return 0, m.GenerateIDErr
+	}
 	m.mu.Lock()
 	defer m.mu.Unlock()
 	m.idCounter++
 	return m.idCounter, nil
 }
 
 func (m *MockStorage) NameToID(name string) (uint32, error) {
+	if m.NameToIDErr != nil {
+		return 0, m.NameToIDErr
+	}
 	m.mu.Lock()
 	defer m.mu.Unlock()
-	if _, exists := m.nameToID[name]; !exists {
-		return 0, errors.New("node with name not found")
+	if id, exists := m.nameToID[name]; exists {
+		return id, nil
 	}
-	return m.nameToID[name], nil
+	return 0, errors.New("node with name not found")
 }
 
 func (m *MockStorage) GetNodes(ids []uint32) (map[uint32]*Node, error) {
+	if m.GetNodesErr != nil {
+		return nil, m.GetNodesErr
+	}
 	m.mu.Lock()
 	defer m.mu.Unlock()
 
@@ -150,18 +204,24 @@ func (m *MockStorage) GetNodes(ids []uint32) (map[uint32]*Node, error) {
 }
 
 func (m *MockStorage) SaveCaches(caches []*NodeCache) error {
+	if m.SaveCachesErr != nil {
+		return m.SaveCachesErr
+	}
 	m.mu.Lock()
 	defer m.mu.Unlock()
+	if m.cache == nil {
+		m.cache = map[uint32]*NodeCache{}
+	}
 	for _, cache := range caches {
-		if m.cache == nil {
-			m.cache = map[uint32]*NodeCache{}
-		}
 		m.cache[cache.ID] = cache
 	}
 	return nil
 }
 
 func (m *MockStorage) GetCaches(ids []uint32) (map[uint32]*NodeCache, error) {
+	if m.GetCachesErr != nil {
+		return nil, m.GetCachesErr
+	}
 	m.mu.Lock()
 	defer m.mu.Unlock()
 
@@ -178,6 +238,9 @@ func (m *MockStorage) GetCaches(ids []uint32) (map[uint32]*NodeCache, error) {
 }
 
 func (m *MockStorage) RemoveAllCaches() error {
+	if m.RemoveAllCachesErr != nil {
+		return m.RemoveAllCachesErr
+	}
 	m.mu.Lock()
 	defer m.mu.Unlock()
 
@@ -192,21 +255,30 @@ func (m *MockStorage) RemoveAllCaches() error {
 	return nil
 }
 
-func (m *MockStorage) AddOrUpdateCustomData(tag, key, datakey string, data []byte) error {
+func (m *MockStorage) AddOrUpdateCustomData(tag, key, dataKey string, data []byte) error {
+	if m.AddOrUpdateCustomDataErr != nil {
+		return m.AddOrUpdateCustomDataErr
+	}
 	m.mu.Lock()
 	defer m.mu.Unlock()
-	if m.db == nil {
-		m.db = make(map[string]map[string][]byte)
-	}
-	if m.db[fmt.Sprintf("%s:%s", tag, key)] == nil {
-		m.db[fmt.Sprintf("%s:%s", tag, key)] = make(map[string][]byte)
+	fullKey := fmt.Sprintf("%s:%s", tag, key)
+	if m.db[fullKey] == nil {
+		m.db[fullKey] = make(map[string][]byte)
 	}
-	m.db[fmt.Sprintf("%s:%s", tag, key)][datakey] = data
+	m.db[fullKey][dataKey] = data
 	return nil
 }
 
 func (m *MockStorage) GetCustomData(tag, key string) (map[string][]byte, error) {
+	if m.GetCustomDataErr != nil {
+		return nil, m.GetCustomDataErr
+	}
 	m.mu.Lock()
 	defer m.mu.Unlock()
-	return m.db[fmt.Sprintf("%s:%s", tag, key)], nil
+	fullKey := fmt.Sprintf("%s:%s", tag, key)
+	data, exists := m.db[fullKey]
+	if !exists {
+		return nil, fmt.Errorf("no data found for tag: %s, key: %s", tag, key)
+	}
+	return data, nil
 }