[WIP] refactor: watch test cleanup #131457
Conversation
27362ea to
4a7f3e7
Compare
|
/assign @wojtek-t |
6a3066b to
027ecc2
Compare
|
@karlkfi Thanks for the refactor. Is it possible to break this huge PR to smaller PRs or even commits? |
Sure. I'll see what i can do. Might have to mostly just be separate commits tho, because making changes to the file causes it to be linted, which requires fixing a lot of different things in the same PR. |
027ecc2 to
410e6e3
Compare
|
Split into three commits for a first pass. This modifies more files tho, which might surface other hidden linter errors. I can probably split the third commit down a bit more, pulling out the sub-tests, close helpers, and testify usage. |
|
/triage accepted |
410e6e3 to
55fb9c7
Compare
|
I split up the commits a bit more. |
|
Surprisingly, the request methods didn't cause additional linter failures by themselves, so i pulled those out to a seperate PR: #131656 You can approve that one first or this whole one. Whichever you prefer. If that one merges first, I'll rebase this one. |
c629d9c to
f99076c
Compare
f99076c to
408f782
Compare
- Add channel test helpers to k8s.io/client-go/util/testing
- Use the new channel test helpers in the client and server watch
tests to validate that the result channels closes without error
when the client stops the watcher.
- Use the new channel test helpers in the client watcher decoder
tests to validate that encoded watch events can be decoded and
that the decoder errors with EOF when stopped asynchronously.
These new tests uncovered existing errors (added TODOs):
1. The watch client doesn't close the response body when it
encounters a NegotiateError.
2. The watch client sometimes sends a decode error on the result
channel after the client watcher has been stopped.
3. StreamWatcher.receive uses %v instead of %w when wrapping errors.
- Wait for storage watcher to be created by the watcher server before trying to send events to it in the tests.
- Use require and assert to make the tests easier to read and debug
408f782 to
3c5acfa
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: karlkfi The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@karlkfi: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| "sync" | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/google/go-cmp/cmp" | ||
| "github.com/stretchr/testify/assert" |
this is not true #130346 , tests are not easier to read and debug |
What type of PR is this?
/kind cleanup
/kind flake
What this PR does / why we need it:
Does this PR introduce a user-facing change?
Dependencies