From c18dd2719eea90959b470b61d6e7806bacb323de Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 16 Nov 2022 19:46:08 +0100 Subject: [PATCH 1/5] cli/compose/loader: TestMarshallConfig: fix duplicate version The version was originally added in 570ee9cb5406e2ec0513e578bc71c730f5e616b6, at the time the `expected` config did not have a `version:` field. A later refactor in 0cf2e6353a88a12b78da72db6a7e7c7d049d3ed8 updated the `expected` config to have a `version:` included. However, the test was not updated, which now resulted in the test using a compose file with a duplicate version field: version: '3.10' version: "3.10" services: foo: build: This issue was masked by `yaml.Unmarshal()` from `gopkg.in/yaml.v2` which silently ignores the duplicate, taking the value of the last occurrence. When upgrading to `gopkg.in/yaml.v3`, the duplicate value resulted in an error: yaml: unmarshal errors: line 2: mapping key "version" already defined at line 1 Signed-off-by: Sebastiaan van Stijn --- cli/compose/loader/types_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cli/compose/loader/types_test.go b/cli/compose/loader/types_test.go index 55e37e97e7..0823509c8e 100644 --- a/cli/compose/loader/types_test.go +++ b/cli/compose/loader/types_test.go @@ -19,8 +19,8 @@ func TestMarshallConfig(t *testing.T) { assert.NilError(t, err) assert.Check(t, is.Equal(expected, string(actual))) - // Make sure the expected still - dict, err := ParseYAML([]byte("version: '3.10'\n" + expected)) + // Make sure the expected can be parsed. + dict, err := ParseYAML([]byte(expected)) assert.NilError(t, err) _, err = Load(buildConfigDetails(dict, map[string]string{})) assert.NilError(t, err) From 0644aa390694aa7c0de35fe6c4e6d153e7d70e4d Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 16 Nov 2022 21:38:32 +0100 Subject: [PATCH 2/5] cli/compose/types: UlimitsConfig.MarshalYAML() fix recursion When marshaling the type with `gopkg.in/yaml.v3`, unmarshaling would recursively call the type's `MarshalYAML()` function, which ultimately resulted in a crash: runtime: goroutine stack exceeds 1000000000-byte limit runtime: sp=0x140202e0430 stack=[0x140202e0000, 0x140402e0000] fatal error: stack overflow This applies a similar fix as was implemented in e7788d6f9a61d67566518efe11e394c414a21173 for the `MarshalJSON()` implementation. An alternative would be to use a type alias (to remove the `MarshalYAML()`), but keeping it simple. Signed-off-by: Sebastiaan van Stijn --- cli/compose/types/types.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cli/compose/types/types.go b/cli/compose/types/types.go index 3ebb75d81d..ed2e10e85b 100644 --- a/cli/compose/types/types.go +++ b/cli/compose/types/types.go @@ -441,7 +441,8 @@ func (u *UlimitsConfig) MarshalYAML() (interface{}, error) { if u.Single != 0 { return u.Single, nil } - return u, nil + // Return as a value to avoid re-entering this method and use the default implementation + return *u, nil } // MarshalJSON makes UlimitsConfig implement json.Marshaller From 5aba4860de4858238b2a50dca053d421911a38e5 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 16 Nov 2022 22:08:09 +0100 Subject: [PATCH 3/5] cli-plugins/manager: TestPluginError: don't use yaml.Marshal The test used `gopkg.in/yaml.v2` to verify the TextMarshaller implementation, which was implemented to allow printing the errors in JSON formatted output; > This exists primarily to implement encoding.TextMarshaller such that > rendering a plugin as JSON (e.g. for `docker info -f '{{json .CLIPlugins}}'`) > renders the Err field as a useful string and not just `{}`. Given that both yaml.Marshal and json.Marshal use this, we may as well use Go's stdlib. While updating, also changed some of the assertions to checks, so that we don't fail the test early. Signed-off-by: Sebastiaan van Stijn --- cli-plugins/manager/error_test.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/cli-plugins/manager/error_test.go b/cli-plugins/manager/error_test.go index fbdb57fd4d..55222d7042 100644 --- a/cli-plugins/manager/error_test.go +++ b/cli-plugins/manager/error_test.go @@ -1,24 +1,24 @@ package manager import ( + "encoding/json" "fmt" "testing" - "github.com/pkg/errors" - "gopkg.in/yaml.v2" "gotest.tools/v3/assert" + is "gotest.tools/v3/assert/cmp" ) func TestPluginError(t *testing.T) { err := NewPluginError("new error") - assert.Error(t, err, "new error") + assert.Check(t, is.Error(err, "new error")) inner := fmt.Errorf("testing") err = wrapAsPluginError(inner, "wrapping") - assert.Error(t, err, "wrapping: testing") - assert.Assert(t, errors.Is(err, inner)) + assert.Check(t, is.Error(err, "wrapping: testing")) + assert.Check(t, is.ErrorIs(err, inner)) - actual, err := yaml.Marshal(err) - assert.NilError(t, err) - assert.Equal(t, "'wrapping: testing'\n", string(actual)) + actual, err := json.Marshal(err) + assert.Check(t, err) + assert.Check(t, is.Equal(`"wrapping: testing"`, string(actual))) } From 4d2fb68b93452f7ddaf7722428d483770bacd2a4 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 16 Nov 2022 22:21:16 +0100 Subject: [PATCH 4/5] cli/compose/loader: fix error messages, and various tests Various fixes: - Don't capitalize error messages - Rename variables that collided with imports or types - Prefer assert.Check over assert.Assert to prevent tests covering multiple cases from failing early - Fix inconsistent order of expected <--> actual, which made it difficult to check which output was the expected output. - Fix formatting of some comments Signed-off-by: Sebastiaan van Stijn --- cli/compose/loader/full-struct_test.go | 12 +-- cli/compose/loader/loader.go | 6 +- cli/compose/loader/loader_test.go | 138 ++++++++++++------------- 3 files changed, 78 insertions(+), 78 deletions(-) diff --git a/cli/compose/loader/full-struct_test.go b/cli/compose/loader/full-struct_test.go index ee70bafe23..17536816df 100644 --- a/cli/compose/loader/full-struct_test.go +++ b/cli/compose/loader/full-struct_test.go @@ -197,7 +197,7 @@ func services(workingDir, homeDir string) []types.ServiceConfig { }, Pid: "host", Ports: []types.ServicePortConfig{ - //"3000", + // "3000", { Mode: "ingress", Target: 3000, @@ -228,14 +228,14 @@ func services(workingDir, homeDir string) []types.ServiceConfig { Target: 3005, Protocol: "tcp", }, - //"8000:8000", + // "8000:8000", { Mode: "ingress", Target: 8000, Published: 8000, Protocol: "tcp", }, - //"9090-9091:8080-8081", + // "9090-9091:8080-8081", { Mode: "ingress", Target: 8080, @@ -248,21 +248,21 @@ func services(workingDir, homeDir string) []types.ServiceConfig { Published: 9091, Protocol: "tcp", }, - //"49100:22", + // "49100:22", { Mode: "ingress", Target: 22, Published: 49100, Protocol: "tcp", }, - //"127.0.0.1:8001:8001", + // "127.0.0.1:8001:8001", { Mode: "ingress", Target: 8001, Published: 8001, Protocol: "tcp", }, - //"127.0.0.1:5000-5010:5000-5010", + // "127.0.0.1:5000-5010:5000-5010", { Mode: "ingress", Target: 5000, diff --git a/cli/compose/loader/loader.go b/cli/compose/loader/loader.go index 203719f509..57a4bffc32 100644 --- a/cli/compose/loader/loader.go +++ b/cli/compose/loader/loader.go @@ -51,7 +51,7 @@ func ParseYAML(source []byte) (map[string]interface{}, error) { } cfgMap, ok := cfg.(map[interface{}]interface{}) if !ok { - return nil, errors.Errorf("Top-level object must be a mapping") + return nil, errors.Errorf("top-level object must be a mapping") } converted, err := convertToStringKeysRecursive(cfgMap, "") if err != nil { @@ -386,9 +386,9 @@ func formatInvalidKeyError(keyPrefix string, key interface{}) error { if keyPrefix == "" { location = "at top level" } else { - location = fmt.Sprintf("in %s", keyPrefix) + location = "in " + keyPrefix } - return errors.Errorf("Non-string key %s: %#v", location, key) + return errors.Errorf("non-string key %s: %#v", location, key) } // LoadServices produces a ServiceConfig map from a compose file Dict diff --git a/cli/compose/loader/loader_test.go b/cli/compose/loader/loader_test.go index cdd214ca1b..1acd16bc2e 100644 --- a/cli/compose/loader/loader_test.go +++ b/cli/compose/loader/loader_test.go @@ -317,13 +317,13 @@ func TestParseAndLoad(t *testing.T) { func TestInvalidTopLevelObjectType(t *testing.T) { _, err := loadYAML("1") - assert.ErrorContains(t, err, "Top-level object must be a mapping") + assert.Check(t, is.ErrorContains(err, "top-level object must be a mapping")) - _, err = loadYAML("\"hello\"") - assert.ErrorContains(t, err, "Top-level object must be a mapping") + _, err = loadYAML(`"hello"`) + assert.Check(t, is.ErrorContains(err, "top-level object must be a mapping")) - _, err = loadYAML("[\"hello\"]") - assert.ErrorContains(t, err, "Top-level object must be a mapping") + _, err = loadYAML(`["hello"]`) + assert.Check(t, is.ErrorContains(err, "top-level object must be a mapping")) } func TestNonStringKeys(t *testing.T) { @@ -333,7 +333,7 @@ version: "3" foo: image: busybox `) - assert.ErrorContains(t, err, "Non-string key at top level: 123") + assert.Check(t, is.ErrorContains(err, "non-string key at top level: 123")) _, err = loadYAML(` version: "3" @@ -343,7 +343,7 @@ services: 123: image: busybox `) - assert.ErrorContains(t, err, "Non-string key in services: 123") + assert.Check(t, is.ErrorContains(err, "non-string key in services: 123")) _, err = loadYAML(` version: "3" @@ -356,7 +356,7 @@ networks: config: - 123: oh dear `) - assert.ErrorContains(t, err, "Non-string key in networks.default.ipam.config[0]: 123") + assert.Check(t, is.ErrorContains(err, "non-string key in networks.default.ipam.config[0]: 123")) _, err = loadYAML(` version: "3" @@ -366,7 +366,7 @@ services: environment: 1: FOO `) - assert.ErrorContains(t, err, "Non-string key in services.dict-env.environment: 1") + assert.Check(t, is.ErrorContains(err, "non-string key in services.dict-env.environment: 1")) } func TestSupportedVersion(t *testing.T) { @@ -376,7 +376,7 @@ services: foo: image: busybox `) - assert.NilError(t, err) + assert.Check(t, err) _, err = loadYAML(` version: "3.0" @@ -384,7 +384,7 @@ services: foo: image: busybox `) - assert.NilError(t, err) + assert.Check(t, err) } func TestUnsupportedVersion(t *testing.T) { @@ -394,7 +394,7 @@ services: foo: image: busybox `) - assert.ErrorContains(t, err, "version") + assert.Check(t, is.ErrorContains(err, "version")) _, err = loadYAML(` version: "2.0" @@ -402,7 +402,7 @@ services: foo: image: busybox `) - assert.ErrorContains(t, err, "version") + assert.Check(t, is.ErrorContains(err, "version")) } func TestInvalidVersion(t *testing.T) { @@ -412,7 +412,7 @@ services: foo: image: busybox `) - assert.ErrorContains(t, err, "version must be a string") + assert.Check(t, is.ErrorContains(err, "version must be a string")) } func TestV1Unsupported(t *testing.T) { @@ -420,7 +420,7 @@ func TestV1Unsupported(t *testing.T) { foo: image: busybox `) - assert.ErrorContains(t, err, "(root) Additional property foo is not allowed") + assert.Check(t, is.ErrorContains(err, "(root) Additional property foo is not allowed")) _, err = loadYAML(` version: "1.0" @@ -428,7 +428,7 @@ foo: image: busybox `) - assert.ErrorContains(t, err, "unsupported Compose file version: 1.0") + assert.Check(t, is.ErrorContains(err, "unsupported Compose file version: 1.0")) } func TestNonMappingObject(t *testing.T) { @@ -438,14 +438,14 @@ services: - foo: image: busybox `) - assert.ErrorContains(t, err, "services must be a mapping") + assert.Check(t, is.ErrorContains(err, "services must be a mapping")) _, err = loadYAML(` version: "3" services: foo: busybox `) - assert.ErrorContains(t, err, "services.foo must be a mapping") + assert.Check(t, is.ErrorContains(err, "services.foo must be a mapping")) _, err = loadYAML(` version: "3" @@ -453,14 +453,14 @@ networks: - default: driver: bridge `) - assert.ErrorContains(t, err, "networks must be a mapping") + assert.Check(t, is.ErrorContains(err, "networks must be a mapping")) _, err = loadYAML(` version: "3" networks: default: bridge `) - assert.ErrorContains(t, err, "networks.default must be a mapping") + assert.Check(t, is.ErrorContains(err, "networks.default must be a mapping")) _, err = loadYAML(` version: "3" @@ -468,14 +468,14 @@ volumes: - data: driver: local `) - assert.ErrorContains(t, err, "volumes must be a mapping") + assert.Check(t, is.ErrorContains(err, "volumes must be a mapping")) _, err = loadYAML(` version: "3" volumes: data: local `) - assert.ErrorContains(t, err, "volumes.data must be a mapping") + assert.Check(t, is.ErrorContains(err, "volumes.data must be a mapping")) } func TestNonStringImage(t *testing.T) { @@ -485,7 +485,7 @@ services: foo: image: ["busybox", "latest"] `) - assert.ErrorContains(t, err, "services.foo.image must be a string") + assert.Check(t, is.ErrorContains(err, "services.foo.image must be a string")) } func TestLoadWithEnvironment(t *testing.T) { @@ -535,7 +535,7 @@ services: environment: FOO: ["1"] `) - assert.ErrorContains(t, err, "services.dict-env.environment.FOO must be a string, number or null") + assert.Check(t, is.ErrorContains(err, "services.dict-env.environment.FOO must be a string, number or null")) } func TestInvalidEnvironmentObject(t *testing.T) { @@ -546,7 +546,7 @@ services: image: busybox environment: "FOO=1" `) - assert.ErrorContains(t, err, "services.dict-env.environment must be a mapping") + assert.Check(t, is.ErrorContains(err, "services.dict-env.environment must be a mapping")) } func TestLoadWithEnvironmentInterpolation(t *testing.T) { @@ -791,17 +791,16 @@ services: // Default behavior keeps the `env_file` entries configWithEnvFiles, err := Load(configDetails) assert.NilError(t, err) - assert.DeepEqual(t, configWithEnvFiles.Services[0].EnvFile, types.StringList{ - "example1.env", - "example2.env", - }) - assert.DeepEqual(t, configWithEnvFiles.Services[0].Environment, expectedEnvironmentMap) + expected := types.StringList{"example1.env", "example2.env"} + assert.Check(t, is.DeepEqual(expected, configWithEnvFiles.Services[0].EnvFile)) + assert.Check(t, is.DeepEqual(expectedEnvironmentMap, configWithEnvFiles.Services[0].Environment)) // Custom behavior removes the `env_file` entries configWithoutEnvFiles, err := Load(configDetails, WithDiscardEnvFiles) assert.NilError(t, err) - assert.DeepEqual(t, configWithoutEnvFiles.Services[0].EnvFile, types.StringList(nil)) - assert.DeepEqual(t, configWithoutEnvFiles.Services[0].Environment, expectedEnvironmentMap) + expected = types.StringList(nil) + assert.Check(t, is.DeepEqual(expected, configWithoutEnvFiles.Services[0].EnvFile)) + assert.Check(t, is.DeepEqual(expectedEnvironmentMap, configWithoutEnvFiles.Services[0].Environment)) } func TestBuildProperties(t *testing.T) { @@ -882,7 +881,7 @@ func TestInvalidResource(t *testing.T) { impossible: x: 1 `) - assert.ErrorContains(t, err, "Additional property impossible is not allowed") + assert.Check(t, is.ErrorContains(err, "Additional property impossible is not allowed")) } func TestInvalidExternalAndDriverCombination(t *testing.T) { @@ -894,8 +893,8 @@ volumes: driver: foobar `) - assert.ErrorContains(t, err, "conflicting parameters \"external\" and \"driver\" specified for volume") - assert.ErrorContains(t, err, "external_volume") + assert.Check(t, is.ErrorContains(err, `conflicting parameters "external" and "driver" specified for volume`)) + assert.Check(t, is.ErrorContains(err, `external_volume`)) } func TestInvalidExternalAndDirverOptsCombination(t *testing.T) { @@ -908,8 +907,8 @@ volumes: beep: boop `) - assert.ErrorContains(t, err, "conflicting parameters \"external\" and \"driver_opts\" specified for volume") - assert.ErrorContains(t, err, "external_volume") + assert.Check(t, is.ErrorContains(err, `conflicting parameters "external" and "driver_opts" specified for volume`)) + assert.Check(t, is.ErrorContains(err, `external_volume`)) } func TestInvalidExternalAndLabelsCombination(t *testing.T) { @@ -922,8 +921,8 @@ volumes: - beep=boop `) - assert.ErrorContains(t, err, "conflicting parameters \"external\" and \"labels\" specified for volume") - assert.ErrorContains(t, err, "external_volume") + assert.Check(t, is.ErrorContains(err, `conflicting parameters "external" and "labels" specified for volume`)) + assert.Check(t, is.ErrorContains(err, `external_volume`)) } func TestLoadVolumeInvalidExternalNameAndNameCombination(t *testing.T) { @@ -936,8 +935,8 @@ volumes: name: external_name `) - assert.ErrorContains(t, err, "volume.external.name and volume.name conflict; only use volume.name") - assert.ErrorContains(t, err, "external_volume") + assert.Check(t, is.ErrorContains(err, "volume.external.name and volume.name conflict; only use volume.name")) + assert.Check(t, is.ErrorContains(err, `external_volume`)) } func durationPtr(value time.Duration) *types.Duration { @@ -954,25 +953,25 @@ func uint32Ptr(value uint32) *uint32 { } func TestFullExample(t *testing.T) { - bytes, err := os.ReadFile("full-example.yml") + data, err := os.ReadFile("full-example.yml") assert.NilError(t, err) homeDir := "/home/foo" env := map[string]string{"HOME": homeDir, "QUX": "qux_from_environment"} - config, err := loadYAMLWithEnv(string(bytes), env) + config, err := loadYAMLWithEnv(string(data), env) assert.NilError(t, err) workingDir, err := os.Getwd() assert.NilError(t, err) - expectedConfig := fullExampleConfig(workingDir, homeDir) + expected := fullExampleConfig(workingDir, homeDir) - assert.Check(t, is.DeepEqual(expectedConfig.Services, config.Services)) - assert.Check(t, is.DeepEqual(expectedConfig.Networks, config.Networks)) - assert.Check(t, is.DeepEqual(expectedConfig.Volumes, config.Volumes)) - assert.Check(t, is.DeepEqual(expectedConfig.Secrets, config.Secrets)) - assert.Check(t, is.DeepEqual(expectedConfig.Configs, config.Configs)) - assert.Check(t, is.DeepEqual(expectedConfig.Extras, config.Extras)) + assert.Check(t, is.DeepEqual(expected.Services, config.Services)) + assert.Check(t, is.DeepEqual(expected.Networks, config.Networks)) + assert.Check(t, is.DeepEqual(expected.Volumes, config.Volumes)) + assert.Check(t, is.DeepEqual(expected.Secrets, config.Secrets)) + assert.Check(t, is.DeepEqual(expected.Configs, config.Configs)) + assert.Check(t, is.DeepEqual(expected.Extras, config.Extras)) } func TestLoadTmpfsVolume(t *testing.T) { @@ -1014,7 +1013,7 @@ services: tmpfs: size: 10000 `) - assert.ErrorContains(t, err, "services.tmpfs.volumes.0 Additional property tmpfs is not allowed") + assert.Check(t, is.ErrorContains(err, "services.tmpfs.volumes.0 Additional property tmpfs is not allowed")) } func TestLoadBindMountSourceMustNotBeEmpty(t *testing.T) { @@ -1027,7 +1026,7 @@ services: - type: bind target: /app `) - assert.Error(t, err, `invalid mount config for type "bind": field Source must not be empty`) + assert.Check(t, is.Error(err, `invalid mount config for type "bind": field Source must not be empty`)) } func TestLoadBindMountSourceIsWindowsAbsolute(t *testing.T) { @@ -1243,8 +1242,9 @@ services: `) assert.NilError(t, err) + expected := samplePortsConfig assert.Check(t, is.Len(config.Services, 1)) - assert.Check(t, is.DeepEqual(samplePortsConfig, config.Services[0].Ports)) + assert.Check(t, is.DeepEqual(expected, config.Services[0].Ports)) } func TestLoadExpandedMountFormat(t *testing.T) { @@ -1334,7 +1334,7 @@ func TestLoadVolumesWarnOnDeprecatedExternalNameVersion34(t *testing.T) { }, }, } - volumes, err := LoadVolumes(source, "3.4") + vols, err := LoadVolumes(source, "3.4") assert.NilError(t, err) expected := map[string]types.VolumeConfig{ "foo": { @@ -1342,7 +1342,7 @@ func TestLoadVolumesWarnOnDeprecatedExternalNameVersion34(t *testing.T) { External: types.External{External: true}, }, } - assert.Check(t, is.DeepEqual(expected, volumes)) + assert.Check(t, is.DeepEqual(expected, vols)) assert.Check(t, is.Contains(buf.String(), "volume.external.name is deprecated")) } @@ -1364,7 +1364,7 @@ func TestLoadVolumesWarnOnDeprecatedExternalNameVersion33(t *testing.T) { }, }, } - volumes, err := LoadVolumes(source, "3.3") + vols, err := LoadVolumes(source, "3.3") assert.NilError(t, err) expected := map[string]types.VolumeConfig{ "foo": { @@ -1372,7 +1372,7 @@ func TestLoadVolumesWarnOnDeprecatedExternalNameVersion33(t *testing.T) { External: types.External{External: true}, }, } - assert.Check(t, is.DeepEqual(expected, volumes)) + assert.Check(t, is.DeepEqual(expected, vols)) assert.Check(t, is.Equal("", buf.String())) } @@ -1432,8 +1432,8 @@ secrets: name: external_name `) - assert.ErrorContains(t, err, "secret.external.name and secret.name conflict; only use secret.name") - assert.ErrorContains(t, err, "external_secret") + assert.Check(t, is.ErrorContains(err, "secret.external.name and secret.name conflict; only use secret.name")) + assert.Check(t, is.ErrorContains(err, "external_secret")) } func TestLoadSecretsWarnOnDeprecatedExternalNameVersion35(t *testing.T) { @@ -1450,7 +1450,7 @@ func TestLoadSecretsWarnOnDeprecatedExternalNameVersion35(t *testing.T) { details := types.ConfigDetails{ Version: "3.5", } - secrets, err := LoadSecrets(source, details) + s, err := LoadSecrets(source, details) assert.NilError(t, err) expected := map[string]types.SecretConfig{ "foo": { @@ -1458,7 +1458,7 @@ func TestLoadSecretsWarnOnDeprecatedExternalNameVersion35(t *testing.T) { External: types.External{External: true}, }, } - assert.Check(t, is.DeepEqual(expected, secrets)) + assert.Check(t, is.DeepEqual(expected, s)) assert.Check(t, is.Contains(buf.String(), "secret.external.name is deprecated")) } @@ -1473,7 +1473,7 @@ func TestLoadNetworksWarnOnDeprecatedExternalNameVersion35(t *testing.T) { }, }, } - networks, err := LoadNetworks(source, "3.5") + nws, err := LoadNetworks(source, "3.5") assert.NilError(t, err) expected := map[string]types.NetworkConfig{ "foo": { @@ -1481,7 +1481,7 @@ func TestLoadNetworksWarnOnDeprecatedExternalNameVersion35(t *testing.T) { External: types.External{External: true}, }, } - assert.Check(t, is.DeepEqual(expected, networks)) + assert.Check(t, is.DeepEqual(expected, nws)) assert.Check(t, is.Contains(buf.String(), "network.external.name is deprecated")) } @@ -1518,8 +1518,8 @@ networks: name: external_name `) - assert.ErrorContains(t, err, "network.external.name and network.name conflict; only use network.name") - assert.ErrorContains(t, err, "foo") + assert.Check(t, is.ErrorContains(err, "network.external.name and network.name conflict; only use network.name")) + assert.Check(t, is.ErrorContains(err, "foo")) } func TestLoadNetworkWithName(t *testing.T) { @@ -1556,7 +1556,7 @@ networks: "network3": {}, }, } - assert.DeepEqual(t, config, expected, cmpopts.EquateEmpty()) + assert.Check(t, is.DeepEqual(expected, config, cmpopts.EquateEmpty())) } func TestLoadInit(t *testing.T) { @@ -1603,7 +1603,7 @@ services: config, err := loadYAML(testcase.yaml) assert.NilError(t, err) assert.Check(t, is.Len(config.Services, 1)) - assert.Check(t, is.DeepEqual(config.Services[0].Init, testcase.init)) + assert.Check(t, is.DeepEqual(testcase.init, config.Services[0].Init)) }) } } @@ -1731,7 +1731,7 @@ secrets: }, }, } - assert.DeepEqual(t, config, expected, cmpopts.EquateEmpty()) + assert.Check(t, is.DeepEqual(expected, config, cmpopts.EquateEmpty())) } func TestLoadSecretDriver(t *testing.T) { @@ -1795,5 +1795,5 @@ secrets: }, }, } - assert.DeepEqual(t, config, expected, cmpopts.EquateEmpty()) + assert.Check(t, is.DeepEqual(expected, config, cmpopts.EquateEmpty())) } From bf3f419b6e3fe79d4da6beec31ab43cde045806c Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 17 Nov 2022 01:11:06 +0100 Subject: [PATCH 5/5] cli/command/stack: TestConfigMergeInterpolation: various fixes - Make the package-level configMergeTests local to the test itself. - Rename fields to better describe intent - Remove some redundant variables - Reverse "expected" and "actual" fields for consistency - Use assert.Check() to not fail early Signed-off-by: Sebastiaan van Stijn --- cli/command/stack/config_test.go | 78 +++++++++++++++----------------- 1 file changed, 36 insertions(+), 42 deletions(-) diff --git a/cli/command/stack/config_test.go b/cli/command/stack/config_test.go index 1fa2f04510..b4f50160fd 100644 --- a/cli/command/stack/config_test.go +++ b/cli/command/stack/config_test.go @@ -17,29 +17,30 @@ func TestConfigWithEmptyComposeFile(t *testing.T) { assert.ErrorContains(t, cmd.Execute(), `Please specify a Compose file`) } -var configMergeTests = []struct { - name string - skipInterpolation bool - first string - second string - merged string -}{ - { - name: "With Interpolation", - skipInterpolation: false, - first: `version: "3.7" +func TestConfigMergeInterpolation(t *testing.T) { + tests := []struct { + name string + skipInterpolation bool + fileOne string + fileTwo string + expected string + }{ + { + name: "With Interpolation", + skipInterpolation: false, + fileOne: `version: "3.7" services: foo: image: busybox:latest command: cat file1.txt `, - second: `version: "3.7" + fileTwo: `version: "3.7" services: foo: image: busybox:${VERSION} command: cat file2.txt `, - merged: `version: "3.7" + expected: `version: "3.7" services: foo: command: @@ -47,23 +48,23 @@ services: - file2.txt image: busybox:1.0 `, - }, - { - name: "Without Interpolation", - skipInterpolation: true, - first: `version: "3.7" + }, + { + name: "Without Interpolation", + skipInterpolation: true, + fileOne: `version: "3.7" services: foo: image: busybox:latest command: cat file1.txt `, - second: `version: "3.7" + fileTwo: `version: "3.7" services: foo: image: busybox:${VERSION} command: cat file2.txt `, - merged: `version: "3.7" + expected: `version: "3.7" services: foo: command: @@ -71,34 +72,27 @@ services: - file2.txt image: busybox:${VERSION} `, - }, -} + }, + } -func TestConfigMergeInterpolation(t *testing.T) { - for _, tt := range configMergeTests { - t.Run(tt.name, func(t *testing.T) { - firstConfig := []byte(tt.first) - secondConfig := []byte(tt.second) + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + firstConfigData, err := loader.ParseYAML([]byte(tc.fileOne)) + assert.Check(t, err) + secondConfigData, err := loader.ParseYAML([]byte(tc.fileTwo)) + assert.Check(t, err) - firstConfigData, err := loader.ParseYAML(firstConfig) - assert.NilError(t, err) - secondConfigData, err := loader.ParseYAML(secondConfig) - assert.NilError(t, err) - - env := map[string]string{ - "VERSION": "1.0", - } - - cfg, err := outputConfig(composetypes.ConfigDetails{ + actual, err := outputConfig(composetypes.ConfigDetails{ ConfigFiles: []composetypes.ConfigFile{ {Config: firstConfigData, Filename: "firstConfig"}, {Config: secondConfigData, Filename: "secondConfig"}, }, - Environment: env, - }, tt.skipInterpolation) - assert.NilError(t, err) - - assert.Equal(t, cfg, tt.merged) + Environment: map[string]string{ + "VERSION": "1.0", + }, + }, tc.skipInterpolation) + assert.Check(t, err) + assert.Equal(t, tc.expected, actual) }) } }