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

Code before:
			hi2: NewHandshakeInfo(mockProvider1, mockProvider1, []matcher.StringMatcher{
				matcher.StringMatcherForTesting(newStringP("bar.com"), nil, nil, nil, nil, false),
			}, false),
			wantMatch: false,
		},
		{
			desc: "same SAN matchers with different content",
			hi1: NewHandshakeInfo(mockProvider1, mockProvider1, []matcher.StringMatcher{
				matcher.StringMatcherForTesting(newStringP("foo.com"), nil, nil, nil, nil, false),
			}, false),
			hi2: NewHandshakeInfo(mockProvider1, mockProvider1, []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",
			hi1:       NewHandshakeInfo(mockProvider1, mockProvider1, nil, true),
			hi2:       NewHandshakeInfo(mockProvider1, mockProvider1, nil, false),
			wantMatch: false,
		},
		{
			desc: "same rootProvider but different mockCertProvider state",
			hi1: &HandshakeInfo{
				rootProvider:      mockProvider1,
				identityProvider:  mockProvider1,
				sanMatchers:       nil,
				requireClientCert: false,
			},
			hi2: &HandshakeInfo{
				rootProvider:      &mockCertProvider{id: 1},
				identityProvider:  mockProvider1,
				sanMatchers:       nil,
				requireClientCert: false,
			},
			wantMatch: false,
		},
	}

	for _, test := range tests {
		t.Run(test.desc, func(t *testing.T) {
			gotMatch := test.hi1.Equal(test.hi2)
			if gotMatch != test.wantMatch {
				t.Errorf("hi1.Equal(hi2) = %v; wantMatch %v", gotMatch, test.wantMatch)
			}
		})
	}
}


Review comment (suggestion):
Technically, the `HandshakeInfo.Equal` method only compares pointers of the certificate providers. See this line:

```golang
	if hi.rootProvider != other.rootProvider ||
		hi.identityProvider != other.identityProvider ||
		hi.requireClientCert != other.requireClientCert ||
		len(hi.sanMatchers) != len(other.sanMatchers) {
		return false
	}
```

So, I don't think we need to have an `id` field within the `testCertProvider` and we can get rid of any test cases which check that.

Code after:
		{
			desc:      "different requireClientCert flags",
			hi1:       NewHandshakeInfo(&testCertProvider{}, &testCertProvider{}, nil, true),
			hi2:       NewHandshakeInfo(&testCertProvider{}, &testCertProvider{}, nil, false),
			wantMatch: false,
		},
		{
			desc:      "same identity provider, different root provider",
			hi1:       NewHandshakeInfo(&testCertProvider{}, testCertProvider{}, nil, false),
			hi2:       NewHandshakeInfo(&testCertProvider{}, testCertProvider{}, nil, false),
			wantMatch: false,
		},
		{
			desc:      "different identity provider, same root provider",
			hi1:       NewHandshakeInfo(testCertProvider{}, &testCertProvider{}, nil, false),
			hi2:       NewHandshakeInfo(testCertProvider{}, &testCertProvider{}, nil, false),
			wantMatch: false,
		},
	}

	for _, test := range tests {
		t.Run(test.desc, func(t *testing.T) {
			if gotMatch := test.hi1.Equal(test.hi2); gotMatch != test.wantMatch {
				t.Errorf("hi1.Equal(hi2) = %v; wantMatch %v", gotMatch, test.wantMatch)
			}
		})
	}
}