Skip to content
Merged
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
21 changes: 11 additions & 10 deletions cli/command/bundlefile/bundlefile_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@ import (
"strings"
"testing"

"github.com/stretchr/testify/assert"
"github.com/gotestyourself/gotestyourself/assert"
is "github.com/gotestyourself/gotestyourself/assert/cmp"
)

func TestLoadFileV01Success(t *testing.T) {
Expand All @@ -25,9 +26,9 @@ func TestLoadFileV01Success(t *testing.T) {
}`)

bundle, err := LoadFile(reader)
assert.NoError(t, err)
assert.Equal(t, "0.1", bundle.Version)
assert.Len(t, bundle.Services, 2)
assert.Check(t, err)
assert.Check(t, is.Equal("0.1", bundle.Version))
assert.Check(t, is.Len(bundle.Services, 2))
}

func TestLoadFileSyntaxError(t *testing.T) {
Expand All @@ -37,7 +38,7 @@ func TestLoadFileSyntaxError(t *testing.T) {
}`)

_, err := LoadFile(reader)
assert.EqualError(t, err, "JSON syntax error at byte 37: invalid character 'u' looking for beginning of value")
assert.Check(t, is.Error(err, "JSON syntax error at byte 37: invalid character 'u' looking for beginning of value"))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is it possible to automate changing these to assert.Error() (gotestyourself/gotest.tools#71), or do you want to keep that for a follow-up ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's possible to automate the change, but it's not the same as gotestyourself/gotest.tools#71.

The current automated migration preserves behaviour, so testify/require -> Assert, testify/assert -> Check.

I can write another migration that converts specific checks to the canonical Assert, but that is a change in behaviour (Fail -> FailNow), so I'd like to do that in a separate PR so that we can more carefully review the change.

}

func TestLoadFileTypeError(t *testing.T) {
Expand All @@ -52,7 +53,7 @@ func TestLoadFileTypeError(t *testing.T) {
}`)

_, err := LoadFile(reader)
assert.EqualError(t, err, "Unexpected type at byte 94. Expected []string but received string.")
assert.Check(t, is.Error(err, "Unexpected type at byte 94. Expected []string but received string."))
}

func TestPrint(t *testing.T) {
Expand All @@ -66,12 +67,12 @@ func TestPrint(t *testing.T) {
},
},
}
assert.NoError(t, Print(&buffer, bundle))
assert.Check(t, Print(&buffer, bundle))
output := buffer.String()
assert.Contains(t, output, "\"Image\": \"image\"")
assert.Contains(t, output,
assert.Check(t, is.Contains(output, "\"Image\": \"image\""))
assert.Check(t, is.Contains(output,
`"Command": [
"echo",
"something"
]`)
]`))
}
15 changes: 8 additions & 7 deletions cli/command/checkpoint/create_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@ import (
"github.com/docker/cli/internal/test"
"github.com/docker/cli/internal/test/testutil"
"github.com/docker/docker/api/types"
"github.com/gotestyourself/gotestyourself/assert"
is "github.com/gotestyourself/gotestyourself/assert/cmp"
"github.com/pkg/errors"
"github.com/stretchr/testify/assert"
)

func TestCheckpointCreateErrors(t *testing.T) {
Expand Down Expand Up @@ -63,10 +64,10 @@ func TestCheckpointCreateWithOptions(t *testing.T) {
cmd.SetArgs([]string{"container-foo", checkpoint})
cmd.Flags().Set("leave-running", "true")
cmd.Flags().Set("checkpoint-dir", "/dir/foo")
assert.NoError(t, cmd.Execute())
assert.Equal(t, "container-foo", containerID)
assert.Equal(t, checkpoint, checkpointID)
assert.Equal(t, "/dir/foo", checkpointDir)
assert.Equal(t, false, exit)
assert.Equal(t, checkpoint, strings.TrimSpace(cli.OutBuffer().String()))
assert.Check(t, cmd.Execute())
assert.Check(t, is.Equal("container-foo", containerID))
assert.Check(t, is.Equal(checkpoint, checkpointID))
assert.Check(t, is.Equal("/dir/foo", checkpointDir))
assert.Check(t, is.Equal(false, exit))
assert.Check(t, is.Equal(checkpoint, strings.TrimSpace(cli.OutBuffer().String())))
}
9 changes: 5 additions & 4 deletions cli/command/checkpoint/list_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,10 @@ import (
"github.com/docker/cli/internal/test"
"github.com/docker/cli/internal/test/testutil"
"github.com/docker/docker/api/types"
"github.com/gotestyourself/gotestyourself/assert"
is "github.com/gotestyourself/gotestyourself/assert/cmp"
"github.com/gotestyourself/gotestyourself/golden"
"github.com/pkg/errors"
"github.com/stretchr/testify/assert"
)

func TestCheckpointListErrors(t *testing.T) {
Expand Down Expand Up @@ -60,8 +61,8 @@ func TestCheckpointListWithOptions(t *testing.T) {
cmd := newListCommand(cli)
cmd.SetArgs([]string{"container-foo"})
cmd.Flags().Set("checkpoint-dir", "/dir/foo")
assert.NoError(t, cmd.Execute())
assert.Equal(t, "container-foo", containerID)
assert.Equal(t, "/dir/foo", checkpointDir)
assert.Check(t, cmd.Execute())
assert.Check(t, is.Equal("container-foo", containerID))
assert.Check(t, is.Equal("/dir/foo", checkpointDir))
golden.Assert(t, cli.OutBuffer().String(), "checkpoint-list-with-options.golden")
}
11 changes: 6 additions & 5 deletions cli/command/checkpoint/remove_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,9 @@ import (
"github.com/docker/cli/internal/test"
"github.com/docker/cli/internal/test/testutil"
"github.com/docker/docker/api/types"
"github.com/gotestyourself/gotestyourself/assert"
is "github.com/gotestyourself/gotestyourself/assert/cmp"
"github.com/pkg/errors"
"github.com/stretchr/testify/assert"
)

func TestCheckpointRemoveErrors(t *testing.T) {
Expand Down Expand Up @@ -58,8 +59,8 @@ func TestCheckpointRemoveWithOptions(t *testing.T) {
cmd := newRemoveCommand(cli)
cmd.SetArgs([]string{"container-foo", "checkpoint-bar"})
cmd.Flags().Set("checkpoint-dir", "/dir/foo")
assert.NoError(t, cmd.Execute())
assert.Equal(t, "container-foo", containerID)
assert.Equal(t, "checkpoint-bar", checkpointID)
assert.Equal(t, "/dir/foo", checkpointDir)
assert.Check(t, cmd.Execute())
assert.Check(t, is.Equal("container-foo", containerID))
assert.Check(t, is.Equal("checkpoint-bar", checkpointID))
assert.Check(t, is.Equal("/dir/foo", checkpointDir))
}
38 changes: 19 additions & 19 deletions cli/command/cli_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,10 @@ import (
"github.com/docker/docker/api"
"github.com/docker/docker/api/types"
"github.com/docker/docker/client"
"github.com/gotestyourself/gotestyourself/assert"
is "github.com/gotestyourself/gotestyourself/assert/cmp"
"github.com/gotestyourself/gotestyourself/fs"
"github.com/pkg/errors"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"golang.org/x/net/context"
)

Expand All @@ -28,15 +28,15 @@ func TestNewAPIClientFromFlags(t *testing.T) {
},
}
apiclient, err := NewAPIClientFromFlags(opts, configFile)
require.NoError(t, err)
assert.Equal(t, host, apiclient.DaemonHost())
assert.NilError(t, err)
assert.Check(t, is.Equal(host, apiclient.DaemonHost()))

expectedHeaders := map[string]string{
"My-Header": "Custom-Value",
"User-Agent": UserAgent(),
}
assert.Equal(t, expectedHeaders, apiclient.(*client.Client).CustomHTTPHeaders())
assert.Equal(t, api.DefaultVersion, apiclient.ClientVersion())
assert.Check(t, is.DeepEqual(expectedHeaders, apiclient.(*client.Client).CustomHTTPHeaders()))
assert.Check(t, is.Equal(api.DefaultVersion, apiclient.ClientVersion()))
}

func TestNewAPIClientFromFlagsWithAPIVersionFromEnv(t *testing.T) {
Expand All @@ -46,20 +46,20 @@ func TestNewAPIClientFromFlagsWithAPIVersionFromEnv(t *testing.T) {
opts := &flags.CommonOptions{}
configFile := &configfile.ConfigFile{}
apiclient, err := NewAPIClientFromFlags(opts, configFile)
require.NoError(t, err)
assert.Equal(t, customVersion, apiclient.ClientVersion())
assert.NilError(t, err)
assert.Check(t, is.Equal(customVersion, apiclient.ClientVersion()))
}

// TODO: use gotestyourself/env.Patch
func patchEnvVariable(t *testing.T, key, value string) func() {
oldValue, ok := os.LookupEnv(key)
require.NoError(t, os.Setenv(key, value))
assert.NilError(t, os.Setenv(key, value))
return func() {
if !ok {
require.NoError(t, os.Unsetenv(key))
assert.NilError(t, os.Unsetenv(key))
return
}
require.NoError(t, os.Setenv(key, oldValue))
assert.NilError(t, os.Setenv(key, oldValue))
}
}

Expand Down Expand Up @@ -125,8 +125,8 @@ func TestInitializeFromClient(t *testing.T) {

cli := &DockerCli{client: apiclient}
cli.initializeFromClient()
assert.Equal(t, testcase.expectedServer, cli.serverInfo)
assert.Equal(t, testcase.negotiated, apiclient.negotiated)
assert.Check(t, is.DeepEqual(testcase.expectedServer, cli.serverInfo))
assert.Check(t, is.Equal(testcase.negotiated, apiclient.negotiated))
})
}
}
Expand Down Expand Up @@ -164,8 +164,8 @@ func TestExperimentalCLI(t *testing.T) {
cli := &DockerCli{client: apiclient, err: os.Stderr}
cliconfig.SetDir(dir.Path())
err := cli.Initialize(flags.NewClientOptions())
assert.NoError(t, err)
assert.Equal(t, testcase.expectedExperimentalCLI, cli.ClientInfo().HasExperimental)
assert.Check(t, err)
assert.Check(t, is.Equal(testcase.expectedExperimentalCLI, cli.ClientInfo().HasExperimental))
})
}
}
Expand Down Expand Up @@ -267,9 +267,9 @@ func TestOrchestratorSwitch(t *testing.T) {
options.Common.Orchestrator = testcase.flagOrchestrator
}
err := cli.Initialize(options)
assert.NoError(t, err)
assert.Equal(t, testcase.expectedKubernetes, cli.ClientInfo().HasKubernetes())
assert.Equal(t, testcase.expectedOrchestrator, string(cli.ClientInfo().Orchestrator))
assert.Check(t, err)
assert.Check(t, is.Equal(testcase.expectedKubernetes, cli.ClientInfo().HasKubernetes()))
assert.Check(t, is.Equal(testcase.expectedOrchestrator, string(cli.ClientInfo().Orchestrator)))
})
}
}
Expand Down Expand Up @@ -335,7 +335,7 @@ func TestGetClientWithPassword(t *testing.T) {
return
}

assert.NoError(t, err)
assert.Check(t, err)
})
}
}
17 changes: 9 additions & 8 deletions cli/command/config/create_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,10 @@ import (
"github.com/docker/cli/internal/test/testutil"
"github.com/docker/docker/api/types"
"github.com/docker/docker/api/types/swarm"
"github.com/gotestyourself/gotestyourself/assert"
is "github.com/gotestyourself/gotestyourself/assert/cmp"
"github.com/gotestyourself/gotestyourself/golden"
"github.com/pkg/errors"
"github.com/stretchr/testify/assert"
)

const configDataFile = "config-create-with-name.golden"
Expand Down Expand Up @@ -70,9 +71,9 @@ func TestConfigCreateWithName(t *testing.T) {

cmd := newConfigCreateCommand(cli)
cmd.SetArgs([]string{name, filepath.Join("testdata", configDataFile)})
assert.NoError(t, cmd.Execute())
assert.Check(t, cmd.Execute())
golden.Assert(t, string(actual), configDataFile)
assert.Equal(t, "ID-"+name, strings.TrimSpace(cli.OutBuffer().String()))
assert.Check(t, is.Equal("ID-"+name, strings.TrimSpace(cli.OutBuffer().String())))
}

func TestConfigCreateWithLabels(t *testing.T) {
Expand All @@ -83,7 +84,7 @@ func TestConfigCreateWithLabels(t *testing.T) {
name := "foo"

data, err := ioutil.ReadFile(filepath.Join("testdata", configDataFile))
assert.NoError(t, err)
assert.Check(t, err)

expected := swarm.ConfigSpec{
Annotations: swarm.Annotations{
Expand All @@ -109,8 +110,8 @@ func TestConfigCreateWithLabels(t *testing.T) {
cmd.SetArgs([]string{name, filepath.Join("testdata", configDataFile)})
cmd.Flags().Set("label", "lbl1=Label-foo")
cmd.Flags().Set("label", "lbl2=Label-bar")
assert.NoError(t, cmd.Execute())
assert.Equal(t, "ID-"+name, strings.TrimSpace(cli.OutBuffer().String()))
assert.Check(t, cmd.Execute())
assert.Check(t, is.Equal("ID-"+name, strings.TrimSpace(cli.OutBuffer().String())))
}

func TestConfigCreateWithTemplatingDriver(t *testing.T) {
Expand Down Expand Up @@ -138,6 +139,6 @@ func TestConfigCreateWithTemplatingDriver(t *testing.T) {
cmd := newConfigCreateCommand(cli)
cmd.SetArgs([]string{name, filepath.Join("testdata", configDataFile)})
cmd.Flags().Set("template-driver", expectedDriver.Name)
assert.NoError(t, cmd.Execute())
assert.Equal(t, "ID-"+name, strings.TrimSpace(cli.OutBuffer().String()))
assert.Check(t, cmd.Execute())
assert.Check(t, is.Equal("ID-"+name, strings.TrimSpace(cli.OutBuffer().String())))
}
8 changes: 4 additions & 4 deletions cli/command/config/inspect_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ import (
// Import builders to get the builder function as package function
. "github.com/docker/cli/internal/test/builders"
"github.com/docker/cli/internal/test/testutil"
"github.com/gotestyourself/gotestyourself/assert"
"github.com/gotestyourself/gotestyourself/golden"
"github.com/stretchr/testify/assert"
)

func TestConfigInspectErrors(t *testing.T) {
Expand Down Expand Up @@ -96,7 +96,7 @@ func TestConfigInspectWithoutFormat(t *testing.T) {
cli := test.NewFakeCli(&fakeClient{configInspectFunc: tc.configInspectFunc})
cmd := newConfigInspectCommand(cli)
cmd.SetArgs(tc.args)
assert.NoError(t, cmd.Execute())
assert.Check(t, cmd.Execute())
golden.Assert(t, cli.OutBuffer().String(), fmt.Sprintf("config-inspect-without-format.%s.golden", tc.name))
}
}
Expand Down Expand Up @@ -133,7 +133,7 @@ func TestConfigInspectWithFormat(t *testing.T) {
cmd := newConfigInspectCommand(cli)
cmd.SetArgs(tc.args)
cmd.Flags().Set("format", tc.format)
assert.NoError(t, cmd.Execute())
assert.Check(t, cmd.Execute())
golden.Assert(t, cli.OutBuffer().String(), fmt.Sprintf("config-inspect-with-format.%s.golden", tc.name))
}
}
Expand Down Expand Up @@ -167,7 +167,7 @@ func TestConfigInspectPretty(t *testing.T) {

cmd.SetArgs([]string{"configID"})
cmd.Flags().Set("pretty", "true")
assert.NoError(t, cmd.Execute())
assert.Check(t, cmd.Execute())
golden.Assert(t, cli.OutBuffer().String(), fmt.Sprintf("config-inspect-pretty.%s.golden", tc.name))
}
}
17 changes: 9 additions & 8 deletions cli/command/config/ls_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,9 @@ import (
// Import builders to get the builder function as package function
. "github.com/docker/cli/internal/test/builders"
"github.com/docker/cli/internal/test/testutil"
"github.com/gotestyourself/gotestyourself/assert"
is "github.com/gotestyourself/gotestyourself/assert/cmp"
"github.com/gotestyourself/gotestyourself/golden"
"github.com/stretchr/testify/assert"
)

func TestConfigListErrors(t *testing.T) {
Expand Down Expand Up @@ -72,7 +73,7 @@ func TestConfigList(t *testing.T) {
},
})
cmd := newConfigListCommand(cli)
assert.NoError(t, cmd.Execute())
assert.Check(t, cmd.Execute())
golden.Assert(t, cli.OutBuffer().String(), "config-list-sort.golden")
}

Expand All @@ -89,7 +90,7 @@ func TestConfigListWithQuietOption(t *testing.T) {
})
cmd := newConfigListCommand(cli)
cmd.Flags().Set("quiet", "true")
assert.NoError(t, cmd.Execute())
assert.Check(t, cmd.Execute())
golden.Assert(t, cli.OutBuffer().String(), "config-list-with-quiet-option.golden")
}

Expand All @@ -108,7 +109,7 @@ func TestConfigListWithConfigFormat(t *testing.T) {
ConfigFormat: "{{ .Name }} {{ .Labels }}",
})
cmd := newConfigListCommand(cli)
assert.NoError(t, cmd.Execute())
assert.Check(t, cmd.Execute())
golden.Assert(t, cli.OutBuffer().String(), "config-list-with-config-format.golden")
}

Expand All @@ -125,15 +126,15 @@ func TestConfigListWithFormat(t *testing.T) {
})
cmd := newConfigListCommand(cli)
cmd.Flags().Set("format", "{{ .Name }} {{ .Labels }}")
assert.NoError(t, cmd.Execute())
assert.Check(t, cmd.Execute())
golden.Assert(t, cli.OutBuffer().String(), "config-list-with-format.golden")
}

func TestConfigListWithFilter(t *testing.T) {
cli := test.NewFakeCli(&fakeClient{
configListFunc: func(options types.ConfigListOptions) ([]swarm.Config, error) {
assert.Equal(t, "foo", options.Filters.Get("name")[0])
assert.Equal(t, "lbl1=Label-bar", options.Filters.Get("label")[0])
assert.Check(t, is.Equal("foo", options.Filters.Get("name")[0]))
assert.Check(t, is.Equal("lbl1=Label-bar", options.Filters.Get("label")[0]))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hm, I find assert.NoError(...) more easy on the eye than assert.Check(....); same as assert.Equal(..); for example:

assert.Equal(t, "lbl1=Label-bar", options.Filters.Get("label")[0])

vs

assert.Check(t, is.Equal("lbl1=Label-bar", options.Filters.Get("label")[0]))

The last one I have to read until position 20 (with indentation, position 32) to see what kind of check is done, and the is.Equal() add another level of nesting, which makes it quite a bit harder to read.

Basically assert.Something( enables me to "scan" the code, and at a glance grasp what steps are taken.

Was this a deliberate design choice in gotestyourself?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I find assert.NoError(...) more easy on the eye than assert.Check(....)

assert.NoError() (in testify) is almost always wrong because it only fails the test, but doesn't end execution immediately. If you don't end execution immediately on an error you will likely hit a panic on a following line. This is one of the many issues that should be fixed.

gotestyourself does have assert.NilError(). The automated migration tool preserves the existing behaviour so we ended up with a few assert.Check(t, err), but these should likely be converted to assert.NilError(t, err) in a follow up.

Was this a deliberate design choice in gotestyourself?

Yes, gotestyourself/gotest.tools#49 (comment).

I think we should add more assert.Something(), but I want to make sure we add the right ones and don't clutter the interface with infrequently used, or weak assertions.

assert.Check(t, is.Equal()) is an interesting case. We need a way to support assertions that use both T.Fail and T.FailNow. There are only so many ways this can be done. testify solved it with code generation and using a separate package. gotestyourself solves it by splitting the failure (assert.Check vs assert.Assert) from the comparison.

The convenience assertions added to the assert package all use FailNow because that should be the more common case. We could add a assert.CheckEqual() but I don't really see that as an improvement over assert.Check(t, isEqual()). Using assert.Equal() (which already exists) should be correct in most cases.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe then you nay use testify/require instead? I think that require.NoError() terminates the execution.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, you could use require.NoError(), but this PR should make it clear that it's too easy to do the wrong thing with testify. It is just one of the many problems with testify. require should be called assert, and assert should be something like "check".

gotestyourself makes it a lot easier to do the right thing by making it the most convenient and fixing the terminology.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@thaJeztah @silvin-lubecki

gotestyourself/gotest.tools#71 adds some more top level asserts for handling errors.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@dnephin looking good! I think we should move this forward (sorry for stalling this). Looks like this'll need a massive rebase (or was it all done automated?)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It was mostly automated, so it should be easy to just re-run that automation.

return []swarm.Config{
*Config(ConfigID("ID-foo"),
ConfigName("foo"),
Expand All @@ -153,6 +154,6 @@ func TestConfigListWithFilter(t *testing.T) {
cmd := newConfigListCommand(cli)
cmd.Flags().Set("filter", "name=foo")
cmd.Flags().Set("filter", "label=lbl1=Label-bar")
assert.NoError(t, cmd.Execute())
assert.Check(t, cmd.Execute())
golden.Assert(t, cli.OutBuffer().String(), "config-list-with-filter.golden")
}
Loading