From 65e4ea27cdaaee448f89efcd7186b2efe89c1b47 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 7 Mar 2022 10:14:15 +0100 Subject: [PATCH 1/3] client: various small test-improvements - avoid accessing non-exported fields where possible, and test using accessors instead, so that we're closer to how it's actually used. - use a variable or const for "expected" in some tests, so that "expected" is printed as part of the test-failure output (instead of just a "value"). - swap the order of "actual" and "expected" for consistency, and to make it easier to see what the "expected" value is in some cases ("expected" on the right, so that it reads `val (actual) != val (expected)`). - don't set fields in the Ping response that are not relevant to the test. - rename some variables for consistency. Signed-off-by: Sebastiaan van Stijn --- client/client_test.go | 107 ++++++++++++++++++++---------------------- 1 file changed, 52 insertions(+), 55 deletions(-) diff --git a/client/client_test.go b/client/client_test.go index 14739f2ca9..4d91dddb1e 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -87,22 +87,21 @@ func TestNewClientWithOpsFromEnv(t *testing.T) { } defer env.PatchAll(t, nil)() - for _, c := range testcases { - env.PatchAll(t, c.envs) - apiclient, err := NewClientWithOpts(FromEnv) - if c.expectedError != "" { - assert.Check(t, is.Error(err, c.expectedError), c.doc) + for _, tc := range testcases { + env.PatchAll(t, tc.envs) + client, err := NewClientWithOpts(FromEnv) + if tc.expectedError != "" { + assert.Check(t, is.Error(err, tc.expectedError), tc.doc) } else { - assert.Check(t, err, c.doc) - version := apiclient.ClientVersion() - assert.Check(t, is.Equal(c.expectedVersion, version), c.doc) + assert.Check(t, err, tc.doc) + assert.Check(t, is.Equal(client.ClientVersion(), tc.expectedVersion), tc.doc) } - if c.envs["DOCKER_TLS_VERIFY"] != "" { + if tc.envs["DOCKER_TLS_VERIFY"] != "" { // pedantic checking that this is handled correctly - tr := apiclient.client.Transport.(*http.Transport) - assert.Assert(t, tr.TLSClientConfig != nil, c.doc) - assert.Check(t, is.Equal(tr.TLSClientConfig.InsecureSkipVerify, false), c.doc) + tr := client.client.Transport.(*http.Transport) + assert.Assert(t, tr.TLSClientConfig != nil, tc.doc) + assert.Check(t, is.Equal(tr.TLSClientConfig.InsecureSkipVerify, false), tc.doc) } } } @@ -127,10 +126,13 @@ func TestGetAPIPath(t *testing.T) { } ctx := context.TODO() - for _, testcase := range testcases { - c := Client{version: testcase.version, basePath: "/"} - actual := c.getAPIPath(ctx, testcase.path, testcase.query) - assert.Check(t, is.Equal(actual, testcase.expected)) + for _, tc := range testcases { + client := Client{ + version: tc.version, + basePath: "/", + } + actual := client.getAPIPath(ctx, tc.path, tc.query) + assert.Check(t, is.Equal(actual, tc.expected)) } } @@ -167,7 +169,7 @@ func TestParseHostURL(t *testing.T) { if testcase.expectedErr != "" { assert.Check(t, is.ErrorContains(err, testcase.expectedErr)) } - assert.Check(t, is.DeepEqual(testcase.expected, actual)) + assert.Check(t, is.DeepEqual(actual, testcase.expected)) } } @@ -183,41 +185,36 @@ func TestNewClientWithOpsFromEnvSetsDefaultVersion(t *testing.T) { if err != nil { t.Fatal(err) } - assert.Check(t, is.Equal(client.version, api.DefaultVersion)) + assert.Check(t, is.Equal(client.ClientVersion(), api.DefaultVersion)) - expected := "1.22" - os.Setenv("DOCKER_API_VERSION", expected) + const expected = "1.22" + _ = os.Setenv("DOCKER_API_VERSION", expected) client, err = NewClientWithOpts(FromEnv) if err != nil { t.Fatal(err) } - assert.Check(t, is.Equal(expected, client.version)) + assert.Check(t, is.Equal(client.ClientVersion(), expected)) } -// TestNegotiateAPIVersionEmpty asserts that client.Client can -// negotiate a compatible APIVersion when omitted +// TestNegotiateAPIVersionEmpty asserts that client.Client version negotiation +// downgrades to the correct API version if the API's ping response does not +// return an API version. func TestNegotiateAPIVersionEmpty(t *testing.T) { defer env.PatchAll(t, map[string]string{"DOCKER_API_VERSION": ""})() client, err := NewClientWithOpts(FromEnv) assert.NilError(t, err) - ping := types.Ping{ - APIVersion: "", - OSType: "linux", - Experimental: false, - } - // set our version to something new client.version = "1.25" // if no version from server, expect the earliest // version before APIVersion was implemented - expected := "1.24" + const expected = "1.24" // test downgrade - client.NegotiateAPIVersionPing(ping) - assert.Check(t, is.Equal(expected, client.version)) + client.NegotiateAPIVersionPing(types.Ping{}) + assert.Equal(t, client.ClientVersion(), expected) } // TestNegotiateAPIVersion asserts that client.Client can @@ -245,28 +242,22 @@ func TestNegotiateAPIVersion(t *testing.T) { expected = "1.20" client.version = expected client.NegotiateAPIVersionPing(ping) - assert.Check(t, is.Equal(expected, client.version)) + assert.Check(t, is.Equal(client.version, expected)) } -// TestNegotiateAPIVersionOverride asserts that we honor -// the environment variable DOCKER_API_VERSION when negotiating versions +// TestNegotiateAPIVersionOverride asserts that we honor the DOCKER_API_VERSION +// environment variable when negotiating versions. func TestNegotiateAPVersionOverride(t *testing.T) { - expected := "9.99" + const expected = "9.99" defer env.PatchAll(t, map[string]string{"DOCKER_API_VERSION": expected})() client, err := NewClientWithOpts(FromEnv) assert.NilError(t, err) - ping := types.Ping{ - APIVersion: "1.24", - OSType: "linux", - Experimental: false, - } - // test that we honored the env var - client.NegotiateAPIVersionPing(ping) - assert.Check(t, is.Equal(expected, client.version)) + client.NegotiateAPIVersionPing(types.Ping{APIVersion: "1.24"}) + assert.Equal(t, client.ClientVersion(), expected) } func TestNegotiateAPIVersionAutomatic(t *testing.T) { @@ -278,24 +269,28 @@ func TestNegotiateAPIVersionAutomatic(t *testing.T) { return resp, nil }) + ctx := context.Background() client, err := NewClientWithOpts( WithHTTPClient(httpClient), WithAPIVersionNegotiation(), ) assert.NilError(t, err) - ctx := context.Background() - assert.Equal(t, client.ClientVersion(), api.DefaultVersion) + // Client defaults to use api.DefaultVersion before version-negotiation. + expected := api.DefaultVersion + assert.Equal(t, client.ClientVersion(), expected) // First request should trigger negotiation pingVersion = "1.35" + expected = "1.35" _, _ = client.Info(ctx) - assert.Equal(t, client.ClientVersion(), "1.35") + assert.Equal(t, client.ClientVersion(), expected) // Once successfully negotiated, subsequent requests should not re-negotiate pingVersion = "1.25" + expected = "1.35" _, _ = client.Info(ctx) - assert.Equal(t, client.ClientVersion(), "1.35") + assert.Equal(t, client.ClientVersion(), expected) } // TestNegotiateAPIVersionWithEmptyVersion asserts that initializing a client @@ -304,18 +299,20 @@ func TestNegotiateAPIVersionWithEmptyVersion(t *testing.T) { client, err := NewClientWithOpts(WithVersion("")) assert.NilError(t, err) - client.NegotiateAPIVersionPing(types.Ping{APIVersion: "1.35"}) - assert.Equal(t, client.version, "1.35") + const expected = "1.35" + client.NegotiateAPIVersionPing(types.Ping{APIVersion: expected}) + assert.Equal(t, client.ClientVersion(), expected) } // TestNegotiateAPIVersionWithFixedVersion asserts that initializing a client -// with an fixed version disables API-version negotiation +// with a fixed version disables API-version negotiation func TestNegotiateAPIVersionWithFixedVersion(t *testing.T) { - client, err := NewClientWithOpts(WithVersion("1.35")) + const customVersion = "1.35" + client, err := NewClientWithOpts(WithVersion(customVersion)) assert.NilError(t, err) client.NegotiateAPIVersionPing(types.Ping{APIVersion: "1.31"}) - assert.Equal(t, client.version, "1.35") + assert.Equal(t, client.ClientVersion(), customVersion) } type roundTripFunc func(*http.Request) (*http.Response, error) @@ -362,13 +359,13 @@ func TestClientRedirect(t *testing.T) { req, err := http.NewRequest(tc.httpMethod, "/redirectme", nil) assert.Check(t, err) resp, err := client.Do(req) - assert.Check(t, is.Equal(tc.statusCode, resp.StatusCode)) + assert.Check(t, is.Equal(resp.StatusCode, tc.statusCode)) if tc.expectedErr == nil { assert.Check(t, is.Nil(err)) } else { urlError, ok := err.(*url.Error) assert.Assert(t, ok, "%T is not *url.Error", err) - assert.Check(t, is.Equal(*tc.expectedErr, *urlError)) + assert.Check(t, is.Equal(*urlError, *tc.expectedErr)) } } } From 25a336ab6afa553954ed59568a35b3950e765a3e Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 7 Mar 2022 11:12:32 +0100 Subject: [PATCH 2/3] client: TestGetAPIPath(): update test to use more realistic results This test was setting the non-exported `Client.basePath` directly, however, it was setting it to a value that would never realistically happen, because `NewClientWithOpts()` initializes the Client with the default API version: https://github.com/moby/moby/blob/ea5b4765d9d9a5aa5cab39f7119cffe74be874ce/client/client.go#L119-L130 Which is used by `getAPIPath()` to construct the URL/path: https://github.com/moby/moby/blob/ea5b4765d9d9a5aa5cab39f7119cffe74be874ce/client/client.go#L176-L190 While this didn't render the test "invalid", using a Client that's constructed in the usual way, makes it more representative. Given that we deprecated (but still support) the non-versioned API paths, with the exception of the `/_ping` API endpoint, we should probably change `getAPIPath()` to default to the "current version", instead of allowing it to use an empty string. Signed-off-by: Sebastiaan van Stijn --- client/client_test.go | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/client/client_test.go b/client/client_test.go index 4d91dddb1e..d2ac64694a 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -113,9 +113,9 @@ func TestGetAPIPath(t *testing.T) { query url.Values expected string }{ - {"", "/containers/json", nil, "/containers/json"}, - {"", "/containers/json", url.Values{}, "/containers/json"}, - {"", "/containers/json", url.Values{"s": []string{"c"}}, "/containers/json?s=c"}, + {"", "/containers/json", nil, "/v" + api.DefaultVersion + "/containers/json"}, + {"", "/containers/json", url.Values{}, "/v" + api.DefaultVersion + "/containers/json"}, + {"", "/containers/json", url.Values{"s": []string{"c"}}, "/v" + api.DefaultVersion + "/containers/json?s=c"}, {"1.22", "/containers/json", nil, "/v1.22/containers/json"}, {"1.22", "/containers/json", url.Values{}, "/v1.22/containers/json"}, {"1.22", "/containers/json", url.Values{"s": []string{"c"}}, "/v1.22/containers/json?s=c"}, @@ -127,10 +127,11 @@ func TestGetAPIPath(t *testing.T) { ctx := context.TODO() for _, tc := range testcases { - client := Client{ - version: tc.version, - basePath: "/", - } + client, err := NewClientWithOpts( + WithVersion(tc.version), + WithHost("tcp://localhost:2375"), + ) + assert.NilError(t, err) actual := client.getAPIPath(ctx, tc.path, tc.query) assert.Check(t, is.Equal(actual, tc.expected)) } From 8512cf076c57e2425501a00e6efc0495c94fee7b Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 7 Mar 2022 11:13:52 +0100 Subject: [PATCH 3/3] client: TestNegotiateAPIVersion(), TestClientRedirect(): use sub-tests Signed-off-by: Sebastiaan van Stijn --- client/client_test.go | 108 +++++++++++++++++++++++++++++------------- 1 file changed, 75 insertions(+), 33 deletions(-) diff --git a/client/client_test.go b/client/client_test.go index d2ac64694a..dfe32dd2f4 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -221,30 +221,69 @@ func TestNegotiateAPIVersionEmpty(t *testing.T) { // TestNegotiateAPIVersion asserts that client.Client can // negotiate a compatible APIVersion with the server func TestNegotiateAPIVersion(t *testing.T) { - client, err := NewClientWithOpts(FromEnv) - assert.NilError(t, err) - - expected := "1.21" - ping := types.Ping{ - APIVersion: expected, - OSType: "linux", - Experimental: false, + tests := []struct { + doc string + clientVersion string + pingVersion string + expectedVersion string + }{ + { + // client should downgrade to the version reported by the daemon. + doc: "downgrade from default", + pingVersion: "1.21", + expectedVersion: "1.21", + }, + { + // client should not downgrade to the version reported by the + // daemon if a custom version was set. + doc: "no downgrade from custom version", + clientVersion: "1.25", + pingVersion: "1.21", + expectedVersion: "1.25", + }, + { + // client should downgrade to the last version before version + // negotiation was added (1.24) if the daemon does not report + // a version. + doc: "downgrade legacy", + pingVersion: "", + expectedVersion: "1.24", + }, + { + // client should downgrade to the version reported by the daemon. + // version negotiation was added in API 1.25, so this is theoretical, + // but it should negotiate to versions before that if the daemon + // gives that as a response. + doc: "downgrade old", + pingVersion: "1.19", + expectedVersion: "1.19", + }, + { + // client should not upgrade to a newer version if a version was set, + // even if both the daemon and the client support it. + doc: "no upgrade", + clientVersion: "1.20", + pingVersion: "1.21", + expectedVersion: "1.20", + }, } - // set our version to something new - client.version = "1.22" - - // test downgrade - client.NegotiateAPIVersionPing(ping) - assert.Check(t, is.Equal(expected, client.version)) - - // set the client version to something older, and verify that we keep the - // original setting. - expected = "1.20" - client.version = expected - client.NegotiateAPIVersionPing(ping) - assert.Check(t, is.Equal(client.version, expected)) - + for _, tc := range tests { + tc := tc + t.Run(tc.doc, func(t *testing.T) { + opts := make([]Opt, 0) + if tc.clientVersion != "" { + // Note that this check is redundant, as WithVersion() considers + // an empty version equivalent to "not setting a version", but + // doing this just to be explicit we are using the default. + opts = append(opts, WithVersion(tc.clientVersion)) + } + client, err := NewClientWithOpts(opts...) + assert.NilError(t, err) + client.NegotiateAPIVersionPing(types.Ping{APIVersion: tc.pingVersion}) + assert.Equal(t, tc.expectedVersion, client.ClientVersion()) + }) + } } // TestNegotiateAPIVersionOverride asserts that we honor the DOCKER_API_VERSION @@ -357,16 +396,19 @@ func TestClientRedirect(t *testing.T) { } for _, tc := range cases { - req, err := http.NewRequest(tc.httpMethod, "/redirectme", nil) - assert.Check(t, err) - resp, err := client.Do(req) - assert.Check(t, is.Equal(resp.StatusCode, tc.statusCode)) - if tc.expectedErr == nil { - assert.Check(t, is.Nil(err)) - } else { - urlError, ok := err.(*url.Error) - assert.Assert(t, ok, "%T is not *url.Error", err) - assert.Check(t, is.Equal(*urlError, *tc.expectedErr)) - } + tc := tc + t.Run(tc.httpMethod, func(t *testing.T) { + req, err := http.NewRequest(tc.httpMethod, "/redirectme", nil) + assert.Check(t, err) + resp, err := client.Do(req) + assert.Check(t, is.Equal(resp.StatusCode, tc.statusCode)) + if tc.expectedErr == nil { + assert.NilError(t, err) + } else { + urlError, ok := err.(*url.Error) + assert.Assert(t, ok, "%T is not *url.Error", err) + assert.Check(t, is.Equal(*urlError, *tc.expectedErr)) + } + }) } }