clusterresolver: remove priority LB related tests #6395
Conversation
| case <-time.After(defaultTestShortTimeout): | ||
| } | ||
|
|
||
| select { |
There was a problem hiding this comment.
This line and below isn't captured in priority_test TestPriority_HighPriorityReady. Can you please copy paste this over there?
There was a problem hiding this comment.
Added a couple of lines in there to check that p0 subConns are still in use, as long as p0 is ready.
| // | ||
| // Init 0 and 1; 0 is up, use 0; 0 is down, 1 is up, use 1; add 2, use 1; 1 is | ||
| // down, use 2; remove 2, use 1. | ||
| func (s) TestEDSPriority_SwitchPriority(t *testing.T) { |
There was a problem hiding this comment.
I know all these tests are a bit clunky, but can you make sure that all the tests deleted don't delete extra operations/assertions that are captured in /xds/internal/balancer/priority/balancer_test.go.
There was a problem hiding this comment.
From my reading of it, it does look like the tests in the priority package cover all scenarios. We definitely will have to rewrite them as e2e style tests. But I'm not planning to hike up that mountain until it becomes a necessity :).
One small hill at a time.
| clab1 := xdstestutils.NewClusterLoadAssignmentBuilder(testClusterNames[0], nil) | ||
| clab1.AddLocality(testSubZones[0], 1, 0, testEndpointAddrs[:1], nil) | ||
| clab1.AddLocality(testSubZones[1], 1, 1, testEndpointAddrs[1:2], nil) | ||
| xdsC.InvokeWatchEDSCallback("", parseEDSRespProtoForTesting(clab1.Build()), nil) |
There was a problem hiding this comment.
Interesting. This test does it through client and lets this balancer build out priority configuration from EDS. Priority_Test just inlines an UpdateClientConnState() with what this balancer would've built out. Yeah, makes more sense to have these tests over there.
There was a problem hiding this comment.
Yeah, we have tests in this package which verify that we build the appropriate priority LB configuration based on received updates. These are unit tests for now, could be changed to an e2e style test with the update coming through the xdsClient, but what we have is good enough for now I think.
These scenarios are all covered in the priority LB policy, and it does not make sense for these tests to be part of the clusterresolver LB policy. Most likely, these tests got left behind when we refactored our LB policies.
#resource-agnostic-xdsclient-api
RELEASE NOTES: none