internal/testutils: add a new test type that implements resolver.ClientConn #6668

easwars · 2023-09-29T15:34:18Z

Summary of changes:

internal/testutils:
- Add a new ResolverClientConn type that implements the resolver.ClientConn interface
- Rename the existing testutils.TestClientConn to testutils.BalancerClientConn and update all references from existing tests
internal/resolver/dns:
- Add a new internal/resolver/dns/internal package to house functionality shared between the dns package and its tests
- Move the fake implementation of the net.Resolver interface used from tests into a separate file. This can do with some cleanup to make it more readable and understandable.
- Move the DNS resolver tests to the dns_test package
- Cleanup the existing DNS resolver tests:
  - Use t.Run wherever possible
  - Use the grpctest.Tester and get rid of direct calls to the leakchecker
  - Get rid of TestMain
  - Introduce helper functions for commonly used functionality

In a follow-up PR, I will using the testutils.ResolverClientConn type from the xDS resolver tests.

#resource-agnostic-xdsclient-api

RELEASE NOTES: none

…ntConn

internal/testutils/resolver.go

codecov · 2023-10-05T18:02:37Z

Codecov Report

❗ No coverage uploaded for pull request base (master@fd9ef72). Click here to learn what that means.
The diff coverage is n/a.

Additional details and impacted files

dfawley · 2023-10-09T15:57:52Z

internal/testutils/resolver.go

+func NewResolverClientConn(t *testing.T, updateState func(resolver.State) error, reportError func(error)) *ResolverClientConn {
+	return &ResolverClientConn{
+		logger:       t,
+		updateStateF: updateState,
+		reportErrorF: reportError,
+	}
+}


Should we just delete this constructor and export the fields? I think it would be a little more ergonomic since the fields would be initialized by name instead of position in the parameter list.

E.g.

rcc := &testutils.ResolverClientConn{ T: t, UpdateStateF: func(...) {}, ReportErrorF: func(...) {}, }

I was initially hesitant to do that because I didn't want to store a testing.T in this type. Ended up exporting the logger interface.

dfawley · 2023-10-09T15:59:09Z

internal/testutils/resolver.go

+// ResolverClientConn is a fake implemetation of the resolver.ClientConn
+// interface to be used in tests.
+type ResolverClientConn struct {
+	resolver.ClientConn // Embedding the interface to avoid implementing deprecated methods.


Can we add something to remind ourselves to remove this in the future?

var _ = ResolverClientConn{}.SomeDeprecatedThing

Hmm ... I don't understand this comment.

Writing something like var _ = ResolverClientConn{}.NewAddress results in an init time panic since this method is not implemented in ResolverClientConn. I initially made a commit which had this and failed to realize that it was causing a panic. So, I had to undo this in the latest commit.

dfawley · 2023-10-10T16:12:19Z

internal/resolver/dns/dns_resolver_test.go

+			target: ":80",
+		},
+		{
+			name:   "",


Add a name?

I honestly don't know what this case is testing. Do you?

dfawley · 2023-10-10T16:15:00Z

internal/resolver/dns/fake_net_resolver_test.go

+	tbl: map[string][]string{
+		"foo.bar.com":          {"1.2.3.4", "5.6.7.8"},
+		"ipv4.single.fake":     {"1.2.3.4"},
+		"srv.ipv4.single.fake": {"2.4.6.8"},
+		"srv.ipv4.multi.fake":  {},
+		"srv.ipv6.single.fake": {},
+		"srv.ipv6.multi.fake":  {},
+		"ipv4.multi.fake":      {"1.2.3.4", "5.6.7.8", "9.10.11.12"},
+		"ipv6.single.fake":     {"2607:f8b0:400a:801::1001"},
+		"ipv6.multi.fake":      {"2607:f8b0:400a:801::1001", "2607:f8b0:400a:801::1002", "2607:f8b0:400a:801::1003"},


Hmm, I was hoping this and the below table would go away in any rewrite. Is there any way to make this declared in a constructor instead? If it's too much work we can punt, but IMO splitting test code that depends on each other into very distant places is a very bad practice.

I will look into this. But if we can do that as part of a follow up PR, that would be good, since many of the other xDS PRs are gated on the change to add testutils.ResolverClientConn.

Improved the host lookup and srv lookup parts. Working on improving the txt lookup.

dfawley · 2023-10-10T16:15:42Z

internal/resolver/dns/fake_net_resolver_test.go

+// resolver functionality, with scfs as the input and scs used for validation of
+// the output. For scfs[3], it corresponds to empty service config, since there
+// isn't a matched choice.
+var scfs = []string{


Similar - these are very magical and nowhere near the code that relies on them.

Same here. Thanks.

dfawley

Approved for the current set of changes modulo:

Delete test case named ""
Add var _ business so we know when to un-embed

easwars · 2023-10-12T02:35:30Z

Approved for the current set of changes modulo:

Delete test case named ""

Done.

Add var _ business so we know when to un-embed

Couldn't add this. The comment thread around this has more details.

…ntConn (grpc#6668)

easwars added 3 commits September 29, 2023 15:22

internal/testutils: add a new test type that implements resolver.Clie…

c68b5c2

…ntConn

multiple: update references to old test balancer.ClientConn

d4698bf

dns: cleanup tests

e5a5650

easwars requested a review from dfawley September 29, 2023 15:34

easwars assigned dfawley Sep 29, 2023

easwars added the Type: Testing label Sep 29, 2023

easwars added this to the 1.59 Release milestone Sep 29, 2023

make vet happy

59fcba5

This was referenced Oct 3, 2023

xds/resolver: move cluster specifier plugin tests to test only package #6681

Merged

xds/resolver: move service watching tests to resolver_test package #6682

Merged

dfawley reviewed Oct 5, 2023

View reviewed changes

internal/testutils/resolver.go Outdated Show resolved Hide resolved

internal/testutils/resolver.go Outdated Show resolved Hide resolved

dfawley assigned easwars and unassigned dfawley Oct 5, 2023

use callbacks instead of channels in ResolverClientConn

f2f715a

easwars assigned dfawley and unassigned easwars Oct 5, 2023

ginayeh modified the milestones: 1.59 Release, 1.60 Release Oct 5, 2023

dfawley reviewed Oct 10, 2023

View reviewed changes

dfawley assigned easwars and unassigned dfawley Oct 10, 2023

easwars added 3 commits October 11, 2023 18:04

get rid of testutils.NewResolverClientConn

2a09a1a

make the fake resolver's host lookup table configurable by the test

6184f19

make the fake resolver's srv lookup table configurable by the test

2eb918f

dfawley approved these changes Oct 11, 2023

View reviewed changes

easwars added 3 commits October 11, 2023 23:42

take care of dfawley's last couple of comments

dbe13ee

make the fake resolver's txt lookup table configurable by the test

508481a

unable to take care of one of dfawley's comments

c794230

arvindbr8 approved these changes Oct 12, 2023

View reviewed changes

arvindbr8 assigned arvindbr8 and easwars and unassigned easwars and arvindbr8 Oct 12, 2023

easwars merged commit dd4c0ad into grpc:master Oct 12, 2023
13 checks passed

easwars deleted the test_resolver_clientconn branch October 12, 2023 17:09

arvindbr8 pushed a commit to arvindbr8/grpc-go that referenced this pull request Nov 7, 2023

internal/testutils: add a new test type that implements resolver.Clie…

a21eedf

…ntConn (grpc#6668)

sebastianjonasson mentioned this pull request May 3, 2024

Delayed dns resolve after upgrade to v1.60.0 #7186

Closed

github-actions bot locked as resolved and limited conversation to collaborators Jun 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal/testutils: add a new test type that implements resolver.ClientConn #6668

internal/testutils: add a new test type that implements resolver.ClientConn #6668

easwars commented Sep 29, 2023 •

edited

Loading

codecov bot commented Oct 5, 2023 •

edited

Loading

dfawley Oct 9, 2023

easwars Oct 11, 2023

easwars Oct 12, 2023

dfawley Oct 9, 2023

easwars Oct 11, 2023

easwars Oct 12, 2023

dfawley Oct 10, 2023

easwars Oct 11, 2023

easwars Oct 12, 2023

dfawley Oct 10, 2023

easwars Oct 11, 2023

easwars Oct 11, 2023

dfawley Oct 10, 2023

easwars Oct 11, 2023

dfawley left a comment

easwars commented Oct 12, 2023

internal/testutils: add a new test type that implements resolver.ClientConn #6668

internal/testutils: add a new test type that implements resolver.ClientConn #6668

Conversation

easwars commented Sep 29, 2023 • edited Loading

codecov bot commented Oct 5, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfawley left a comment

Choose a reason for hiding this comment

easwars commented Oct 12, 2023

easwars commented Sep 29, 2023 •

edited

Loading

codecov bot commented Oct 5, 2023 •

edited

Loading