Merge pull request #43342 from thaJeztah/client_test_cleanup

client: cleanup and fix some tests

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)
 		}
 	}
 }
@@ -114,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 +126,14 @@ 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, 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))
 	}
 }
 
@@ -167,7 +170,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,90 +186,118 @@ 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
 // 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(expected, client.version))
-
+	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 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 +309,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 +339,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)
@@ -359,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(tc.statusCode, resp.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))
-		}
+		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))
+			}
+		})
 	}
 }