Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/cdi/cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,7 @@ func (c *Cache) RemoveSpec(name string) error {

specDir, _ = c.highestPrioritySpecDir()
if specDir == "" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elezar Can this still happen with #310 merged ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't be possible, no.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that mean we want to update the highestPrioritySpecDir implementation to not return an error?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it does not return an error. It returns a directory and its priority.

return errors.New("no Spec directories to remove from")
return fmt.Errorf("no Spec directories defined: %w", fs.ErrNotExist)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the fix looks good to me, but how about adding test case for this?

err := cache.RemoveSpec("something")
assert.True(t, errors.Is(err, fs.ErrNotExist))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a basic test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@klihub as also pointed out by the test, we do have a slight disconnect here. In the case where the spec dirs are empty, we return ErrNotExist, but in the case where os.Remove fails for the spec we are trying to remove due to ErrNotExist we return nil. Should we always return nil (or ErrNotExist) for consistency?

}

path = filepath.Join(specDir, name)
Expand Down
40 changes: 40 additions & 0 deletions pkg/cdi/cache_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1839,6 +1839,46 @@ devices:
}
}

func TestRemoveCache(t *testing.T) {

tmpDir := t.TempDir()

testCases := []struct {
name string
cache *Cache
specToRemove string
expectedError error
}{
{
name: "returns error if cache dirs is empty",
cache: &Cache{
specDirs: nil,
},
specToRemove: "any",
expectedError: os.ErrNotExist,
},
{
name: "returns no error if spec does not exist",
cache: &Cache{
specDirs: []string{tmpDir},
},
specToRemove: "non-existent",
expectedError: nil,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
err := tc.cache.RemoveSpec(tc.specToRemove)
if tc.expectedError == nil {
require.NoError(t, err)
} else {
require.ErrorIs(t, err, tc.expectedError)
}
})
}
}

// Create and populate automatically cleaned up spec directories.
func createSpecDirs(t *testing.T, etc, run map[string]string) (string, error) {
return mkTestDir(t, map[string]map[string]string{
Expand Down