From 65e4ea27cdaaee448f89efcd7186b2efe89c1b47 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 7 Mar 2022 10:14:15 +0100 Subject: [PATCH] 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)) } } }