From 7c8b5708ebea29ee325045e265fad8257c08f737 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 5 Mar 2018 19:59:53 -0500 Subject: [PATCH 1/2] Don't set a default filename for ConfigFile With a default filename tests will leave a file in the working directory that is never cleaned up. Signed-off-by: Daniel Nephin --- cli/command/registry/login_test.go | 41 +++++++++++++++++------------- cli/config/config_test.go | 9 +++---- internal/test/cli.go | 14 +++++----- 3 files changed, 35 insertions(+), 29 deletions(-) diff --git a/cli/command/registry/login_test.go b/cli/command/registry/login_test.go index 11db1ba6b0..afb929e2c8 100644 --- a/cli/command/registry/login_test.go +++ b/cli/command/registry/login_test.go @@ -5,14 +5,14 @@ import ( "fmt" "testing" - "golang.org/x/net/context" - "github.com/docker/cli/internal/test" "github.com/docker/docker/api/types" registrytypes "github.com/docker/docker/api/types/registry" "github.com/docker/docker/client" "github.com/gotestyourself/gotestyourself/assert" is "github.com/gotestyourself/gotestyourself/assert/cmp" + "github.com/gotestyourself/gotestyourself/fs" + "golang.org/x/net/context" ) const userErr = "userunknownError" @@ -131,22 +131,27 @@ func TestRunLogin(t *testing.T) { expectedErr: testAuthErrMsg, }, } - for _, tc := range testCases { - cli := test.NewFakeCli(&fakeClient{}) - errBuf := new(bytes.Buffer) - cli.SetErr(errBuf) - if tc.inputStoredCred != nil { - cred := *tc.inputStoredCred - cli.ConfigFile().GetCredentialsStore(cred.ServerAddress).Store(cred) - } - loginErr := runLogin(cli, tc.inputLoginOption) - if tc.expectedErr != "" { - assert.Check(t, is.Equal(tc.expectedErr, loginErr.Error())) - } else { - assert.Check(t, loginErr) - savedCred, credStoreErr := cli.ConfigFile().GetCredentialsStore(tc.inputStoredCred.ServerAddress).Get(tc.inputStoredCred.ServerAddress) + for i, tc := range testCases { + t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { + tmpFile := fs.NewFile(t, "test-run-login") + defer tmpFile.Remove() + cli := test.NewFakeCli(&fakeClient{}) + configfile := cli.ConfigFile() + configfile.Filename = tmpFile.Path() + + if tc.inputStoredCred != nil { + cred := *tc.inputStoredCred + configfile.GetCredentialsStore(cred.ServerAddress).Store(cred) + } + loginErr := runLogin(cli, tc.inputLoginOption) + if tc.expectedErr != "" { + assert.Error(t, loginErr, tc.expectedErr) + return + } + assert.NilError(t, loginErr) + savedCred, credStoreErr := configfile.GetCredentialsStore(tc.inputStoredCred.ServerAddress).Get(tc.inputStoredCred.ServerAddress) assert.Check(t, credStoreErr) - assert.Check(t, is.DeepEqual(tc.expectedSavedCred, savedCred)) - } + assert.DeepEqual(t, tc.expectedSavedCred, savedCred) + }) } } diff --git a/cli/config/config_test.go b/cli/config/config_test.go index ca036ea0d6..1314abe666 100644 --- a/cli/config/config_test.go +++ b/cli/config/config_test.go @@ -10,7 +10,6 @@ import ( "github.com/docker/cli/cli/config/configfile" "github.com/docker/cli/cli/config/credentials" - "github.com/docker/cli/internal/test/testutil" "github.com/docker/docker/pkg/homedir" "github.com/gotestyourself/gotestyourself/assert" is "github.com/gotestyourself/gotestyourself/assert/cmp" @@ -78,7 +77,7 @@ func TestEmptyFile(t *testing.T) { assert.NilError(t, err) _, err = Load(tmpHome) - testutil.ErrorContains(t, err, "EOF") + assert.ErrorContains(t, err, "EOF") } func TestEmptyJSON(t *testing.T) { @@ -122,7 +121,7 @@ email`: "Invalid auth configuration file", assert.NilError(t, err) _, err = Load(tmpHome) - testutil.ErrorContains(t, err, expectedError) + assert.ErrorContains(t, err, expectedError) } } @@ -469,7 +468,7 @@ func TestJSONSaveWithNoFile(t *testing.T) { config, err := LoadFromReader(strings.NewReader(js)) assert.NilError(t, err) err = config.Save() - testutil.ErrorContains(t, err, "with empty filename") + assert.ErrorContains(t, err, "with empty filename") tmpHome, err := ioutil.TempDir("", "config-test") assert.NilError(t, err) @@ -500,7 +499,7 @@ func TestLegacyJSONSaveWithNoFile(t *testing.T) { config, err := LegacyLoadFromReader(strings.NewReader(js)) assert.NilError(t, err) err = config.Save() - testutil.ErrorContains(t, err, "with empty filename") + assert.ErrorContains(t, err, "with empty filename") tmpHome, err := ioutil.TempDir("", "config-test") assert.NilError(t, err) diff --git a/internal/test/cli.go b/internal/test/cli.go index ad9e1b1489..cac803712c 100644 --- a/internal/test/cli.go +++ b/internal/test/cli.go @@ -40,12 +40,14 @@ func NewFakeCli(client client.APIClient) *FakeCli { outBuffer := new(bytes.Buffer) errBuffer := new(bytes.Buffer) return &FakeCli{ - client: client, - out: command.NewOutStream(outBuffer), - outBuffer: outBuffer, - err: errBuffer, - in: command.NewInStream(ioutil.NopCloser(strings.NewReader(""))), - configfile: configfile.New("configfile"), + client: client, + out: command.NewOutStream(outBuffer), + outBuffer: outBuffer, + err: errBuffer, + in: command.NewInStream(ioutil.NopCloser(strings.NewReader(""))), + // Use an empty string for filename so that tests don't create configfiles + // Set cli.ConfigFile().Filename to a tempfile to support Save. + configfile: configfile.New(""), } } From 789acb526c865f8d0ab22bccd1f59fb150841889 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 6 Mar 2018 13:44:06 -0500 Subject: [PATCH 2/2] Cleanup config load error handling Signed-off-by: Daniel Nephin --- cli/config/config.go | 10 +++++----- cli/config/config_test.go | 5 ++++- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/cli/config/config.go b/cli/config/config.go index 90529ebd4c..143436b9ec 100644 --- a/cli/config/config.go +++ b/cli/config/config.go @@ -75,18 +75,18 @@ func Load(configDir string) (*configfile.ConfigFile, error) { if _, err := os.Stat(filename); err == nil { file, err := os.Open(filename) if err != nil { - return configFile, errors.Errorf("%s - %v", filename, err) + return configFile, errors.Wrap(err, filename) } defer file.Close() err = configFile.LoadFromReader(file) if err != nil { - err = errors.Errorf("%s - %v", filename, err) + err = errors.Wrap(err, filename) } return configFile, err } else if !os.IsNotExist(err) { // if file is there but we can't stat it for any reason other // than it doesn't exist then stop - return configFile, errors.Errorf("%s - %v", filename, err) + return configFile, errors.Wrap(err, filename) } // Can't find latest config file so check for the old one @@ -96,12 +96,12 @@ func Load(configDir string) (*configfile.ConfigFile, error) { } file, err := os.Open(confFile) if err != nil { - return configFile, errors.Errorf("%s - %v", confFile, err) + return configFile, errors.Wrap(err, confFile) } defer file.Close() err = configFile.LegacyLoadFromReader(file) if err != nil { - return configFile, errors.Errorf("%s - %v", confFile, err) + return configFile, errors.Wrap(err, confFile) } return configFile, nil } diff --git a/cli/config/config_test.go b/cli/config/config_test.go index 1314abe666..74c441a4b4 100644 --- a/cli/config/config_test.go +++ b/cli/config/config_test.go @@ -2,6 +2,7 @@ package config import ( "bytes" + "io" "io/ioutil" "os" "path/filepath" @@ -13,6 +14,7 @@ import ( "github.com/docker/docker/pkg/homedir" "github.com/gotestyourself/gotestyourself/assert" is "github.com/gotestyourself/gotestyourself/assert/cmp" + "github.com/pkg/errors" ) func setupConfigDir(t *testing.T) (string, func()) { @@ -77,7 +79,8 @@ func TestEmptyFile(t *testing.T) { assert.NilError(t, err) _, err = Load(tmpHome) - assert.ErrorContains(t, err, "EOF") + assert.Equal(t, errors.Cause(err), io.EOF) + assert.ErrorContains(t, err, ConfigFileName) } func TestEmptyJSON(t *testing.T) {