Skip to content

xds: generic xds client ads stream tests #8307

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
May 15, 2025
Merged

xds: generic xds client ads stream tests #8307

merged 6 commits into from
May 15, 2025

Conversation

purnesh42H
Copy link
Contributor

RELEASE NOTES: None

@purnesh42H purnesh42H requested review from easwars and dfawley May 9, 2025 15:59
@purnesh42H purnesh42H added Type: Testing Area: xDS Includes everything xDS related, including LB policies used with xDS. labels May 9, 2025
@purnesh42H purnesh42H added this to the 1.73 Release milestone May 9, 2025
@codecov Codecov
Copy link

codecov bot commented May 9, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 81.25000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 82.30%. Comparing base (d00f4ac) to head (19248d1).
Report is 15 commits behind head on master.

Files with missing lines Patch % Lines
xds/internal/clients/xdsclient/ads_stream.go 73.91% 4 Missing and 2 partials ⚠️
xds/internal/clients/internal/testutils/channel.go 75.00% 3 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #8307      +/-   ##
==========================================
+ Coverage   82.22%   82.30%   +0.07%     
==========================================
  Files         419      419              
  Lines       41954    42025      +71     
==========================================
+ Hits        34497    34589      +92     
+ Misses       5995     5979      -16     
+ Partials     1462     1457       -5
Files with missing lines Coverage Δ
xds/internal/clients/xdsclient/xdsclient.go 80.25% <100.00%> (+2.05%) ⬆️
xds/internal/clients/internal/testutils/channel.go 89.28% <75.00%> (-10.72%) ⬇️
xds/internal/clients/xdsclient/ads_stream.go 79.77% <73.91%> (+0.14%) ⬆️

... and 29 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

dfawley
dfawley reviewed May 9, 2025
View reviewed changes
xds/internal/clients/xdsclient/xdsclient.go Outdated
@@ -123,6 +123,12 @@ func (c *XDSClient) SetWatchExpiryTimeoutForTesting(watchExpiryTimeout time.Dura
c.watchExpiryTimeout = watchExpiryTimeout
}

// SetStreamBackOffForTesting override the default stream backoff function
// with provided function.
func (c *XDSClient) SetStreamBackOffForTesting(streamBackoff func(int) time.Duration) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please put this in an internal subdirectory and set it to an unexported function here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done for both expiry and backoff

@purnesh42H purnesh42H force-pushed the generic-xds-client-ads-stream-tests branch from 900a1bf to ffe6b75 Compare May 10, 2025 04:17
@purnesh42H purnesh42H requested a review from dfawley May 10, 2025 04:17
@purnesh42H purnesh42H force-pushed the generic-xds-client-ads-stream-tests branch from ffe6b75 to b9afcfa Compare May 10, 2025 04:22
@purnesh42H purnesh42H force-pushed the generic-xds-client-ads-stream-tests branch from b9afcfa to 72bdea5 Compare May 10, 2025 04:24
dfawley
dfawley reviewed May 12, 2025
View reviewed changes
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am going to defer to @easwars to review this since he has a lot more experience/familiarity with the original xdsclient and its tests.

xds/internal/clients/xdsclient/ads_stream_watch_test.go Outdated
@@ -0,0 +1,275 @@
/*
*
* Copyright 2024 gRPC authors.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this moved from elsewhere? You have several "new" files with 2024, so unless they're copies, please update.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah this is copied from internal and then modified. Similar to how done in previous PRs

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. That should make it a lot easier to review @easwars.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests needs to be deleted from internal xdsclient as part of migration PR #8310 as they need to override internal resource watching implementation.

@dfawley dfawley assigned purnesh42H and unassigned dfawley May 12, 2025
@purnesh42H purnesh42H removed their assignment May 12, 2025
easwars
easwars reviewed May 12, 2025
View reviewed changes
@easwars easwars assigned purnesh42H and unassigned easwars May 12, 2025
@purnesh42H purnesh42H mentioned this pull request May 13, 2025
Closed
10 tasks
@purnesh42H purnesh42H requested review from easwars and dfawley May 13, 2025 17:18
@purnesh42H purnesh42H assigned easwars and unassigned purnesh42H May 13, 2025
@easwars easwars assigned purnesh42H and unassigned easwars May 13, 2025
@purnesh42H purnesh42H assigned easwars and unassigned purnesh42H May 14, 2025
@easwars easwars assigned purnesh42H and unassigned easwars May 14, 2025
@purnesh42H purnesh42H force-pushed the generic-xds-client-ads-stream-tests branch from 3956e5b to 19248d1 Compare May 15, 2025 03:52
@purnesh42H purnesh42H merged commit 1ecde18 into grpc:master May 15, 2025
23 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@easwars easwars easwars approved these changes

@dfawley dfawley Awaiting requested review from dfawley

Assignees

@purnesh42H purnesh42H

Labels
Area: xDS Includes everything xDS related, including LB policies used with xDS. Type: Testing
Projects
None yet
Milestone
1.73 Release
Development

Successfully merging this pull request may close these issues.
3 participants
@purnesh42H @easwars @dfawley