Repository: grpc/grpc-go
File: internal/credentials/xds/handshake_info_test.go (Go)

Code before:
	tests := []struct {
		desc      string
		hi1       *HandshakeInfo
		hi2       *HandshakeInfo
		wantMatch bool
	}{
		{
			desc:      "both HandshakeInfo are nil",
			hi1:       nil,
			hi2:       nil,
			wantMatch: true,
		},
		{
			desc:      "one HandshakeInfo is nil",
			hi1:       nil,
			hi2:       NewHandshakeInfo(testCertProvider{}, nil, nil, false),
			wantMatch: false,
		},
		{
			desc: "same providers, same SAN matchers",
			hi1: NewHandshakeInfo(testCertProvider{}, testCertProvider{}, []matcher.StringMatcher{
				matcher.StringMatcherForTesting(newStringP("foo.com"), nil, nil, nil, nil, false),
			}, false),
			hi2: NewHandshakeInfo(testCertProvider{}, testCertProvider{}, []matcher.StringMatcher{
				matcher.StringMatcherForTesting(newStringP("foo.com"), nil, nil, nil, nil, false),
			}, false),
			wantMatch: true,
		},
		{
			desc: "same providers, different SAN matchers",
			hi1: NewHandshakeInfo(testCertProvider{}, testCertProvider{}, []matcher.StringMatcher{
				matcher.StringMatcherForTesting(newStringP("foo.com"), nil, nil, nil, nil, false),
			}, false),
			hi2: NewHandshakeInfo(testCertProvider{}, testCertProvider{}, []matcher.StringMatcher{
				matcher.StringMatcherForTesting(newStringP("bar.com"), nil, nil, nil, nil, false),
			}, false),
			wantMatch: false,
		},
		{
			desc: "same SAN matchers with different content",
			hi1: NewHandshakeInfo(testCertProvider{}, testCertProvider{}, []matcher.StringMatcher{
				matcher.StringMatcherForTesting(newStringP("foo.com"), nil, nil, nil, nil, false),
			}, false),
			hi2: NewHandshakeInfo(testCertProvider{}, testCertProvider{}, []matcher.StringMatcher{
				matcher.StringMatcherForTesting(newStringP("foo.com"), nil, nil, nil, nil, false),
				matcher.StringMatcherForTesting(newStringP("bar.com"), nil, nil, nil, nil, false),
			}, false),
			wantMatch: false,
		},
		{
			desc:      "different requireClientCert flags",

Review comment (suggestion):
This is now a little bit tricky.

`NewHandshakeInfo` is defined to accept two `certprovider.Provider` arguments. `certprovider.Provider` is an interface type, and under the hood an interface is implemented as a tuple of `(value, type)`. See: https://go.dev/tour/methods/11. We are passing a concrete type which is `struct testCertProvider`. And since the concrete type being passed as the parameter is a value type and not a pointer type, the equality comparison in `HandshakeInfo.Equal()` succeeds. This is fine with me. But the problem now is that we don't have a test case for the providers being different.

I'd like to see two more test cases:
1. Case where the identity provider is the same, but the root provider is different
2. Case where the identity provider is different, but the root provider is the same

You could pass a pointer type as a parameter to `NewHandshakeInfo`. So, a `HandshakeInfo` created as `NewHandshakeInfo(&testCertProvider{}, testCertProvider, matchers, false)` will not be equal to another `HandshakeInfo` created as `NewHandshakeInfo(&testCertProvider{}, testCertProvider, matchers, false)` because the first parameter now is a pointer type and therefore will not be considered as equal.




Code after:
	tests := []struct {
		desc      string
		hi1       *HandshakeInfo
		hi2       *HandshakeInfo
		wantMatch bool
	}{
		{
			desc:      "both HandshakeInfo are nil",
			hi1:       nil,
			hi2:       nil,
			wantMatch: true,
		},
		{
			desc:      "one HandshakeInfo is nil",
			hi1:       nil,
			hi2:       NewHandshakeInfo(&testCertProvider{}, nil, nil, false),
			wantMatch: false,
		},
		{
			desc:      "different root providers",
			hi1:       NewHandshakeInfo(&testCertProvider{}, nil, nil, false),
			hi2:       NewHandshakeInfo(&testCertProvider{}, nil, nil, false),
			wantMatch: false,
		},
		{
			desc: "same providers, same SAN matchers",
			hi1: NewHandshakeInfo(testCertProvider{}, testCertProvider{}, []matcher.StringMatcher{
				matcher.StringMatcherForTesting(newStringP("foo.com"), nil, nil, nil, nil, false),
			}, false),
			hi2: NewHandshakeInfo(testCertProvider{}, testCertProvider{}, []matcher.StringMatcher{
				matcher.StringMatcherForTesting(newStringP("foo.com"), nil, nil, nil, nil, false),
			}, false),
			wantMatch: true,
		},
		{
			desc: "same providers, different SAN matchers",
			hi1: NewHandshakeInfo(testCertProvider{}, testCertProvider{}, []matcher.StringMatcher{
				matcher.StringMatcherForTesting(newStringP("foo.com"), nil, nil, nil, nil, false),
			}, false),
			hi2: NewHandshakeInfo(testCertProvider{}, testCertProvider{}, []matcher.StringMatcher{
				matcher.StringMatcherForTesting(newStringP("bar.com"), nil, nil, nil, nil, false),
			}, false),
			wantMatch: false,
		},
		{
			desc: "same SAN matchers with different content",
			hi1: NewHandshakeInfo(&testCertProvider{}, &testCertProvider{}, []matcher.StringMatcher{
				matcher.StringMatcherForTesting(newStringP("foo.com"), nil, nil, nil, nil, false),
			}, false),
			hi2: NewHandshakeInfo(&testCertProvider{}, &testCertProvider{}, []matcher.StringMatcher{
				matcher.StringMatcherForTesting(newStringP("foo.com"), nil, nil, nil, nil, false),