From 0dd6f7f1b33bb6791bb4b22e84918bf7ff3b2510 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 22 Oct 2024 10:58:29 +0200 Subject: [PATCH 1/2] cil/config/credentials: remove newStore() test-utility This function was names slightly confusing, as it returns a fakeStore, and it didn't do any constructing, so didn't provide value above just constructing the type. I'm planning to add more functionality to the fakeStore, but don't want to maintain a full-fledged constructor for all of that, so let's remove this utility. Signed-off-by: Sebastiaan van Stijn --- cli/config/credentials/file_store_test.go | 18 +++++------- cli/config/credentials/native_store_test.go | 32 ++++++++++----------- 2 files changed, 23 insertions(+), 27 deletions(-) diff --git a/cli/config/credentials/file_store_test.go b/cli/config/credentials/file_store_test.go index 94e505e13c..077d89dc8e 100644 --- a/cli/config/credentials/file_store_test.go +++ b/cli/config/credentials/file_store_test.go @@ -24,12 +24,8 @@ func (f *fakeStore) GetFilename() string { return "/tmp/docker-fakestore" } -func newStore(auths map[string]types.AuthConfig) store { - return &fakeStore{configs: auths} -} - func TestFileStoreAddCredentials(t *testing.T) { - f := newStore(make(map[string]types.AuthConfig)) + f := &fakeStore{configs: map[string]types.AuthConfig{}} s := NewFileStore(f) auth := types.AuthConfig{ @@ -47,13 +43,13 @@ func TestFileStoreAddCredentials(t *testing.T) { } func TestFileStoreGet(t *testing.T) { - f := newStore(map[string]types.AuthConfig{ + f := &fakeStore{configs: map[string]types.AuthConfig{ "https://example.com": { Auth: "super_secret_token", Email: "foo@example.com", ServerAddress: "https://example.com", }, - }) + }} s := NewFileStore(f) a, err := s.Get("https://example.com") @@ -71,7 +67,7 @@ func TestFileStoreGet(t *testing.T) { func TestFileStoreGetAll(t *testing.T) { s1 := "https://example.com" s2 := "https://example2.example.com" - f := newStore(map[string]types.AuthConfig{ + f := &fakeStore{configs: map[string]types.AuthConfig{ s1: { Auth: "super_secret_token", Email: "foo@example.com", @@ -82,7 +78,7 @@ func TestFileStoreGetAll(t *testing.T) { Email: "foo@example2.com", ServerAddress: "https://example2.example.com", }, - }) + }} s := NewFileStore(f) as, err := s.GetAll() @@ -107,13 +103,13 @@ func TestFileStoreGetAll(t *testing.T) { } func TestFileStoreErase(t *testing.T) { - f := newStore(map[string]types.AuthConfig{ + f := &fakeStore{configs: map[string]types.AuthConfig{ "https://example.com": { Auth: "super_secret_token", Email: "foo@example.com", ServerAddress: "https://example.com", }, - }) + }} s := NewFileStore(f) err := s.Erase("https://example.com") diff --git a/cli/config/credentials/native_store_test.go b/cli/config/credentials/native_store_test.go index 5abcca3587..f55d269ed5 100644 --- a/cli/config/credentials/native_store_test.go +++ b/cli/config/credentials/native_store_test.go @@ -91,7 +91,7 @@ func mockCommandFn(args ...string) client.Program { } func TestNativeStoreAddCredentials(t *testing.T) { - f := newStore(make(map[string]types.AuthConfig)) + f := &fakeStore{configs: map[string]types.AuthConfig{}} s := &nativeStore{ programFunc: mockCommandFn, fileStore: NewFileStore(f), @@ -116,7 +116,7 @@ func TestNativeStoreAddCredentials(t *testing.T) { } func TestNativeStoreAddInvalidCredentials(t *testing.T) { - f := newStore(make(map[string]types.AuthConfig)) + f := &fakeStore{configs: map[string]types.AuthConfig{}} s := &nativeStore{ programFunc: mockCommandFn, fileStore: NewFileStore(f), @@ -132,11 +132,11 @@ func TestNativeStoreAddInvalidCredentials(t *testing.T) { } func TestNativeStoreGet(t *testing.T) { - f := newStore(map[string]types.AuthConfig{ + f := &fakeStore{configs: map[string]types.AuthConfig{ validServerAddress: { Email: "foo@example.com", }, - }) + }} s := &nativeStore{ programFunc: mockCommandFn, fileStore: NewFileStore(f), @@ -154,11 +154,11 @@ func TestNativeStoreGet(t *testing.T) { } func TestNativeStoreGetIdentityToken(t *testing.T) { - f := newStore(map[string]types.AuthConfig{ + f := &fakeStore{configs: map[string]types.AuthConfig{ validServerAddress2: { Email: "foo@example2.com", }, - }) + }} s := &nativeStore{ programFunc: mockCommandFn, @@ -176,11 +176,11 @@ func TestNativeStoreGetIdentityToken(t *testing.T) { } func TestNativeStoreGetAll(t *testing.T) { - f := newStore(map[string]types.AuthConfig{ + f := &fakeStore{configs: map[string]types.AuthConfig{ validServerAddress: { Email: "foo@example.com", }, - }) + }} s := &nativeStore{ programFunc: mockCommandFn, @@ -217,11 +217,11 @@ func TestNativeStoreGetAll(t *testing.T) { } func TestNativeStoreGetMissingCredentials(t *testing.T) { - f := newStore(map[string]types.AuthConfig{ + f := &fakeStore{configs: map[string]types.AuthConfig{ validServerAddress: { Email: "foo@example.com", }, - }) + }} s := &nativeStore{ programFunc: mockCommandFn, @@ -232,11 +232,11 @@ func TestNativeStoreGetMissingCredentials(t *testing.T) { } func TestNativeStoreGetInvalidAddress(t *testing.T) { - f := newStore(map[string]types.AuthConfig{ + f := &fakeStore{configs: map[string]types.AuthConfig{ validServerAddress: { Email: "foo@example.com", }, - }) + }} s := &nativeStore{ programFunc: mockCommandFn, @@ -247,11 +247,11 @@ func TestNativeStoreGetInvalidAddress(t *testing.T) { } func TestNativeStoreErase(t *testing.T) { - f := newStore(map[string]types.AuthConfig{ + f := &fakeStore{configs: map[string]types.AuthConfig{ validServerAddress: { Email: "foo@example.com", }, - }) + }} s := &nativeStore{ programFunc: mockCommandFn, @@ -263,11 +263,11 @@ func TestNativeStoreErase(t *testing.T) { } func TestNativeStoreEraseInvalidAddress(t *testing.T) { - f := newStore(map[string]types.AuthConfig{ + f := &fakeStore{configs: map[string]types.AuthConfig{ validServerAddress: { Email: "foo@example.com", }, - }) + }} s := &nativeStore{ programFunc: mockCommandFn, From 3c78069240cf69713ffabe1e0212c02e0766de53 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 22 Oct 2024 12:11:36 +0200 Subject: [PATCH 2/2] cli/config/credentials: add test for save being idempotent Test case for d3f6867e4d7f5018ae4c0fbc709934893f0e95a2 Signed-off-by: Sebastiaan van Stijn --- cli/config/credentials/file_store.go | 3 +- cli/config/credentials/file_store_test.go | 79 ++++++++++++++++++++++- 2 files changed, 80 insertions(+), 2 deletions(-) diff --git a/cli/config/credentials/file_store.go b/cli/config/credentials/file_store.go index 07918a2c34..c69312b014 100644 --- a/cli/config/credentials/file_store.go +++ b/cli/config/credentials/file_store.go @@ -28,7 +28,8 @@ func NewFileStore(file store) Store { return &fileStore{file: file} } -// Erase removes the given credentials from the file store. +// Erase removes the given credentials from the file store.This function is +// idempotent and does not update the file if credentials did not change. func (c *fileStore) Erase(serverAddress string) error { if _, exists := c.file.GetAuthConfigs()[serverAddress]; !exists { // nothing to do; no credentials found for the given serverAddress diff --git a/cli/config/credentials/file_store_test.go b/cli/config/credentials/file_store_test.go index 077d89dc8e..466fa054a0 100644 --- a/cli/config/credentials/file_store_test.go +++ b/cli/config/credentials/file_store_test.go @@ -10,9 +10,15 @@ import ( type fakeStore struct { configs map[string]types.AuthConfig + saveFn func(*fakeStore) error } func (f *fakeStore) Save() error { + if f.saveFn != nil { + // Pass a reference to the fakeStore itself in case saveFn + // wants to access it. + return f.saveFn(f) + } return nil } @@ -21,7 +27,78 @@ func (f *fakeStore) GetAuthConfigs() map[string]types.AuthConfig { } func (f *fakeStore) GetFilename() string { - return "/tmp/docker-fakestore" + return "no-config.json" +} + +// TestFileStoreIdempotent verifies that the config-file isn't updated +// if nothing changed. +func TestFileStoreIdempotent(t *testing.T) { + var saveCount, expectedSaveCount int + + s := NewFileStore(&fakeStore{ + configs: map[string]types.AuthConfig{}, + saveFn: func(*fakeStore) error { + saveCount++ + return nil + }, + }) + authOne := types.AuthConfig{ + Auth: "super_secret_token", + Email: "foo@example.com", + ServerAddress: "https://example.com", + } + authTwo := types.AuthConfig{ + Auth: "also_super_secret_token", + Email: "bar@example.com", + ServerAddress: "https://other.example.com", + } + + expectedSaveCount = 1 + t.Run("store new credentials", func(t *testing.T) { + assert.NilError(t, s.Store(authOne)) + retrievedAuth, err := s.Get(authOne.ServerAddress) + assert.NilError(t, err) + assert.Check(t, is.Equal(retrievedAuth, authOne)) + assert.Check(t, is.Equal(saveCount, expectedSaveCount)) + }) + t.Run("store same credentials is a no-op", func(t *testing.T) { + assert.NilError(t, s.Store(authOne)) + retrievedAuth, err := s.Get(authOne.ServerAddress) + assert.NilError(t, err) + assert.Check(t, is.Equal(retrievedAuth, authOne)) + assert.Check(t, is.Equal(saveCount, expectedSaveCount), "should not have saved if nothing changed") + }) + t.Run("store other credentials", func(t *testing.T) { + expectedSaveCount++ + assert.NilError(t, s.Store(authTwo)) + retrievedAuth, err := s.Get(authTwo.ServerAddress) + assert.NilError(t, err) + assert.Check(t, is.Equal(retrievedAuth, authTwo)) + assert.Check(t, is.Equal(saveCount, expectedSaveCount)) + }) + t.Run("erase credentials", func(t *testing.T) { + expectedSaveCount++ + assert.NilError(t, s.Erase(authOne.ServerAddress)) + retrievedAuth, err := s.Get(authOne.ServerAddress) + assert.NilError(t, err) + assert.Check(t, is.Equal(retrievedAuth, types.AuthConfig{})) + assert.Check(t, is.Equal(saveCount, expectedSaveCount)) + }) + t.Run("erase non-existing credentials is a no-op", func(t *testing.T) { + assert.NilError(t, s.Erase(authOne.ServerAddress)) + retrievedAuth, err := s.Get(authOne.ServerAddress) + assert.NilError(t, err) + assert.Check(t, is.Equal(retrievedAuth, types.AuthConfig{})) + assert.Check(t, is.Equal(saveCount, expectedSaveCount), "should not have saved if nothing changed") + }) + t.Run("erase other credentials", func(t *testing.T) { + expectedSaveCount++ + assert.NilError(t, s.Erase(authTwo.ServerAddress)) + retrievedAuth, err := s.Get(authTwo.ServerAddress) + assert.NilError(t, err) + assert.Check(t, is.Equal(retrievedAuth, types.AuthConfig{})) + assert.Check(t, is.Equal(saveCount, expectedSaveCount)) + }) } func TestFileStoreAddCredentials(t *testing.T) {