xdsclient: make load reporting tests e2e style (3/N) #7694
Conversation
easwars
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7694 +/- ##
==========================================
+ Coverage 81.80% 81.89% +0.08%
==========================================
Files 361 361
Lines 27827 27827
==========================================
+ Hits 22765 22790 +25
+ Misses 3861 3844 -17
+ Partials 1201 1193 -8 |
| for { | ||
| u, err := lrsServer.LRSRequestChan.Receive(ctx) | ||
| if err != nil { | ||
| t.Fatalf("Timeout when reading LRS request: %v", err) |
There was a problem hiding this comment.
Similar comment to other PR about the error case when in a polling loop. I think the correct failure for this whole polling loop, perhaps not a failure in this specific operation is that it's a timeout for server waiting for a stream canceled error. I'd like to see that logic weaved in somehow.
There was a problem hiding this comment.
Changed the error message here. As I mentioned in the similar comment on the other PR, this check will fail when the test timeout expires.
Also, changed the call on line 243 to be t.Fatalf. So, if the server sees any other error code, the test will fail immediately.
There was a problem hiding this comment.
Yeah, I just felt like it should be a distinct error/string for failure when the polling loop expires, and not leave it to fail in an operation within the polling loop that is a layer under (i.e. this timeout when reading LRS request failures).
There was a problem hiding this comment.
Yeah your diff is what I was going for. I wish there was a way to wrap it fully, where the whole polling loop if it hits timeout will fail with that error rather than failing with this error, but it's minor and this single conditional add is simple and does the job mostly well.
| // with different server configurations | ||
| // - canceling the load reporting from the client results in the LRS stream | ||
| // being canceled on the server | ||
| func (s) TestReportLoad_ConnectionCreation(t *testing.T) { |
There was a problem hiding this comment.
Do we not need any failure test cases like "Handling errors returned from the server during load reporting by client"?
|
Still LGTM. |
easwars
force-pushed
the
loadreport_tests
branch
from
2ef78dd to
408884b
Compare
This PR moves load reporting tests from the
transportandxdsclientpackage to live in thexdsclient/testspackage and makes them e2e style.#a71-xds-fallback
#xdsclient-refactor
Addresses #6902
RELEASE NOTES: none