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 <github@gone.nl>

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))
 		}
 	}
 }