There is a single caller for this. Prefer getting rid of the function and inlining it here.

The reason I had this in a separate function is I would have to do a bool local.

Switched to that. Let me know what you think.

You don't need a boolean. If you change the body of the for to return when you see grpc.lb.pick_first.disconnections (instead of breaking), then the only case where you will execute code below the for would be when the context expires. And therefore you can check for ctx.Err() != nil instead of checking for the boolean.

If I return early I don't even need the ctx.Err check right? The only reason it would hit that would be for the ctx to expire without having found disconnection/returned right?

So my failure mode is an uncondtional t.Fatalf("timeout waiting for grpc.lb.pick_first.disconnections metric").

Actually, with Arjan's point about await state I can use that to poll/sync and then the metrics test can be deterministic one time check. Thanks Arjan for suggestion.

I think we should have tests for happy eyeballs cases. Did you consider enhancing existing tests to check for metric values at the end of the test? Going down that path would cover a lot of scenarios instead of very simple ones being tested here.

I never considered that. Would you rather me write my own happy eyeballs or scale up the existing? I know there was lots of previous contention about adding too many assertions to tests.

That's a good idea. I would suggest adding assertions to the same test because asserting both behaviours in the same test would give a better indication that the features work together. If this severely hampers readability, we can have separate tests.

Agree with @arjan-bal.

I'm OK with it being a separate PR as well.

Scaled up two basic happy eyeballs tests (in a separate commit).

Note that there is a limitation in our test metrics recorder that it only persist the most recently emitted metric value for a given metric name. The reason for this is vast non determinism in other unit tests that use this component, where the most recent was deterministic but not the total metric. It's too far down the rabbit hole now to change I think, but let me know if y'all feel strongly about trying to add a summation assertion/if you want me to add metrics assertions to more happy eyeballs tests/what you think about the additional assertions in general.

I'll be happy to add more, remove the commit, leave as is, etc.

I added a test to the other test that test with a ClientConn, so I think that should be good enough outside the summation thing, which I don't think should be addressed in this PR if at all unless found assertions inadequate.

I saw other tests test interleaving addresses by sending TF, but figured this coverage should be good enough. Thanks for suggestion.

This one is suffixed with Metric and the others are not. Let's be consistent.

Good point. Done. The RLS/WRR metrics are also suffixed with Metric, just checked.

This comment seems like it's unnecessary or in the wrong place? It's not being dereferenced here.

This was Easwar's suggestion after our back and forth: #7839 (comment). What do you think about this. I'll drop "so guaranteed to be non nil" from this one because it's documented on the field.

+1 to something like:

if oldState == Connecting {
  // A known issue causes a race that prevents the READY state change notification.  This works around it.

Also it would be great if we could create an issue for that problem and link it here so we know to come back and delete this one it's resolved.

Removed second conditional. Created a Github issue and linked to it.

That's a good idea. I would suggest adding assertions to the same test because asserting both behaviours in the same test would give a better indication that the features work together. If this severely hampers readability, we can have separate tests.