xds/resolver: move cluster specifier plugin tests to test only package #6681
Conversation
| func (s) TestRegister(t *testing.T) { | ||
| if resolver.Get(xdsresolver.Scheme) == nil { | ||
| t.Errorf("Scheme %q is not registered", xdsresolver.Scheme) | ||
| } | ||
| } |
There was a problem hiding this comment.
I find this test strange since I've never seen anything testing the registry from init() calls. I feel like other tests can fail as a result of this not failing which is how I've previously called this, but I don't feel strongly about it.
There was a problem hiding this comment.
Yeah, I would say it isn't very common to have this test. But I don't think it hurts to have it either.
I feel like other tests can fail as a result of this not failing
I don't understand why/how other tests would fail as a result of this test. Other tests do read from the registry, and so, at that point, this is a redundant test for sure.
| builder := resolver.Get(xdsresolver.Scheme) | ||
| if builder == nil { | ||
| t.Fatalf("Scheme %q is not registered", xdsresolver.Scheme) | ||
| } |
There was a problem hiding this comment.
I.e. here related to the test comment above
There was a problem hiding this comment.
Thought about it a little more and got rid of TestRegister. Doesn't seem to add much value at all. It if could act as a gate to other tests, i.e failure of this test would prevent other tests from running, then it would be a useful thing to have. Now, if the read from the registry fails, then TestRegister would fail and so would all the other tests. So, it makes sense to get rid of it.
| func (s) TestRegister(t *testing.T) { | ||
| if resolver.Get(xdsScheme) == nil { | ||
| t.Errorf("scheme %v is not registered", xdsScheme) | ||
| if resolver.Get(Scheme) == nil { | ||
| t.Errorf("scheme %v is not registered", Scheme) | ||
| } | ||
| } |
There was a problem hiding this comment.
I guess it was already in codebase.
| } | ||
| } | ||
|
|
||
| // Waits for the resolver to push an update to the fake resolver.ClientConn and |
There was a problem hiding this comment.
Optional nit: verifyUpdateFromResolver waits
easwars
force-pushed
the
xds_resolver_cleanup_2
branch
from
e8264b2 to
4901fb9
Compare
Summary of changes:
xds/internal/resolver/cluster_specifier_plugin_test.gotoresolver_testpackage, thereby making them not rely on any internal detailsresolverpackageString()method on theresolver.Targettype for better logging output#resource-agnostic-xdsclient-api
RELEASE NOTES: none