Skip to content
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

*: Added End-to End Style Test Coverage for LRS Scenarios in xDS Client #7973

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

RyanBlaney
Copy link

Addresses #7704

This pull request adds test coverage for Load Reporting Service (LRS) scenarios within the xdsclient package. The tests address various cases, such as stream failures, backoff mechanisms, and resource request handling when streams are unavailable.

Highlights:

  • Test Coverage:

    • Adds three backoff tests for LRS, modeled after the existing Aggregated Discovery Service (ADS) backoff tests.
    • Validates behavior under edge cases, including stream errors and retries.
    • Ensures proper handling of resource requests when streams are temporarily unavailable.
  • Consistency:

    • Aligns LRS behavior testing with established ADS test patterns to maintain consistency.

Release Notes:

  • Adds test coverage for LRS stream backoff and error scenarios.

Request for Feedback

  1. WatchListener Refinements:

    • Suggestions for improving the WatchListener implementation to better support LRS.
    • Potential approaches for introducing a dedicated LRS-specific watcher for enhanced clarity and modularity.
  2. Test Isolation:

    • Recommendations for isolating load-report test cases from broader listener tests to improve test readability and maintainability.
  3. Error Simulation:

    • Feedback on the approach to simulating edge cases, particularly around stream errors and retries, to ensure robust test coverage.

Copy link

codecov bot commented Dec 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.20%. Comparing base (4c07bca) to head (399a844).
Report is 36 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7973      +/-   ##
==========================================
+ Coverage   81.84%   82.20%   +0.35%     
==========================================
  Files         377      381       +4     
  Lines       38120    38539     +419     
==========================================
+ Hits        31201    31682     +481     
+ Misses       5603     5551      -52     
+ Partials     1316     1306      -10     

see 58 files with indirect coverage changes

@RyanBlaney RyanBlaney marked this pull request as ready for review December 29, 2024 22:30
@purnesh42H purnesh42H self-assigned this Jan 2, 2025
@@ -59,8 +59,10 @@ func init() {
grpc.EnableTracing = false
}

type connCtxKey struct{}
type rpcCtxKey struct{}
type (
Copy link
Contributor

Choose a reason for hiding this comment

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

are the stats_test changes? Please revert the file if unrelated

v3discoverypb "github.com/envoyproxy/go-control-plane/envoy/service/discovery/v3"
)

// Tests the case where the management server returns an error in the ADS
Copy link
Contributor

Choose a reason for hiding this comment

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

copy paste error: ADS Stream -> LRS Stream

resourceRequestCh := make(chan []string, 1)
backoffCh := make(chan struct{}, 1)

// Context with timeout for the test.
Copy link
Contributor

Choose a reason for hiding this comment

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

no need of this comment. Code is self explainatory

client := createXDSClientWithBackoff(t, bc, streamBackoff)

// Start watching a resource.
const listenerName = "listener"
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are writing tests for LRS stream which is used for load reporting, we don't need watchers here. What we need is for client to report load and verify if the server's channel received the LRS request.

Can you take a look at how tests are written and how the lrs server setup here https://github.com/grpc/grpc-go/blob/master/xds/internal/xdsclient/tests/loadreport_test.go ? This will give you some idea on how to go about it

Copy link
Author

Choose a reason for hiding this comment

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

Okay, I had a feeling this was a mistake. Thanks, I'll get this fixed.

@purnesh42H
Copy link
Contributor

@RyanBlaney the tests you have written are for ADS stream which is sending discover requests. For this we need e2e tests for LRS stream which sends load requests to management server. Please write similar tests as here https://github.com/grpc/grpc-go/blob/master/xds/internal/xdsclient/tests/loadreport_test.go

@purnesh42H purnesh42H assigned RyanBlaney and unassigned purnesh42H Jan 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants