-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
xds: switching to stubserver in tests instead of testservice implementation #7726
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7726 +/- ##
==========================================
- Coverage 81.71% 81.66% -0.05%
==========================================
Files 374 374
Lines 38166 37978 -188
==========================================
- Hits 31188 31016 -172
+ Misses 5699 5643 -56
- Partials 1279 1319 +40 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@easwars for second review |
xds/server_ext_test.go
Outdated
server, err := xds.NewGRPCServer(grpc.Creds(insecure.NewCredentials()), modeChangeOpt, xds.BootstrapContentsForTesting(bootstrapContents)) | ||
if err != nil { | ||
t.Fatalf("Failed to create an xDS enabled gRPC server: %v", err) | ||
} | ||
defer server.Stop() | ||
testgrpc.RegisterTestServiceServer(server, &testService{}) | ||
|
||
stub.S = server | ||
stubserver.StartTestService(t, stub) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
combined server initialization and passed sopts to StartTestService.
@janardhankrishna-sai -- Also please mention the PR description why does this diff "partially" fix the issue. The last few comments in #7291 is also not very clear. |
5bec96e
to
05e54a4
Compare
05e54a4
to
1b4f2f4
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the changes per conversations. I just had a few more comments, please take a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, modulo couple of suggestions
t.Cleanup(func() { | ||
stub.S.Stop() | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also work
t.Cleanup(func() { | |
stub.S.Stop() | |
}) | |
t.Cleanup(stub.S.Stop) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done with the change.
fs, nodeID, port, cleanup := clientSetup(t) | ||
defer cleanup() | ||
fs, nodeID, port := clientSetup(t) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed empty line
Partially Addresses: #7291
RELEASE NOTES: None