Skip to content

Navigation Menu

Sign in

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

[WIP] refactor: watch test cleanup #131457

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
Open
wants to merge 4 commits into
base: master
Choose a base branch
Loading
from
Open

[WIP] refactor: watch test cleanup #131457

wants to merge 4 commits into from

Conversation

karlkfi
Copy link
Contributor

@karlkfi karlkfi commented Apr 25, 2025 •
edited
Loading

What type of PR is this?

/kind cleanup
/kind flake

What this PR does / why we need it:

This change fixes a bunch of linter errors in the watch tests.

  • Close response body after http calls
  • Add apitesting utility methods for closing and testing errors from response body and websocket.
  • Validate read after close errors.
  • Use testify require and assert to make the tests easier to read and debug
  • Fix flaky watch tests by waiting for storage watcher to be created by the watcher server before trying to send events to it in the tests.
  • Use sub-tests to avoid side-effects between test cases, cancel the context between cases, and make it easier to determine the failing case in the test logs.
  • Use anonymous closures in benchmarks, instead of sub-tests, so the times still include all cases.

Does this PR introduce a user-facing change?

NONE

Dependencies

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. kind/flake Categorizes issue or PR as related to a flaky test. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Apr 25, 2025
@k8s-ci-robot k8s-ci-robot requested review from deads2k and jpbetz April 25, 2025 03:43
@k8s-ci-robot k8s-ci-robot added area/apiserver sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 25, 2025
@karlkfi karlkfi mentioned this pull request Apr 25, 2025
Open
@karlkfi karlkfi force-pushed the karl-fake-watcher-wait branch from 27362ea to 4a7f3e7 Compare April 26, 2025 05:29
@karlkfi
Copy link
Contributor Author

karlkfi commented Apr 26, 2025

/assign @wojtek-t

@karlkfi karlkfi force-pushed the karl-fake-watcher-wait branch from 4a7f3e7 to 6a3066b Compare April 26, 2025 06:07
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: karlkfi
Once this PR has been reviewed and has the lgtm label, please ask for approval from wojtek-t. For more information see the Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@karlkfi karlkfi force-pushed the karl-fake-watcher-wait branch from 6a3066b to 027ecc2 Compare April 26, 2025 09:11
@siyuanfoundation
Copy link
Contributor

@karlkfi Thanks for the refactor. Is it possible to break this huge PR to smaller PRs or even commits?

@karlkfi
Copy link
Contributor Author

karlkfi commented Apr 29, 2025

@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.

@karlkfi karlkfi force-pushed the karl-fake-watcher-wait branch from 027ecc2 to 410e6e3 Compare April 30, 2025 00:36
@karlkfi
Copy link
Contributor Author

karlkfi commented Apr 30, 2025

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.

@yongruilin
Copy link
Contributor

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels May 1, 2025
@karlkfi karlkfi force-pushed the karl-fake-watcher-wait branch from 410e6e3 to 55fb9c7 Compare May 1, 2025 22:49
@karlkfi
Copy link
Contributor Author

karlkfi commented May 1, 2025

I split up the commits a bit more.

@karlkfi karlkfi mentioned this pull request May 1, 2025
Open
@karlkfi karlkfi mentioned this pull request May 7, 2025
Merged
@karlkfi
Copy link
Contributor Author

karlkfi commented May 7, 2025

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.

@karlkfi karlkfi force-pushed the karl-fake-watcher-wait branch 2 times, most recently from 142d6c9 to c629d9c Compare May 9, 2025 18:29
karlkfi added 4 commits May 9, 2025 11:34
@karlkfi
test: Use sub-tests in watch tests
4b21aa0 
- Use sub-tests to avoid side-effects between test cases,
  cancel the context between cases, and make it easier to
  determine the failing case in the test logs.
- Use anonymous closures in benchmarks, instead of sub-tests,
  so the times still include all cases.
@karlkfi
test: Fix flaky watch tests
d0bdab0 
- Wait for storage watcher to be created by the watcher server
  before trying to send events to it in the tests.
@karlkfi
test: use testify in watch tests
08bbc5b 
- Use require and assert to make the tests easier to read and debug
@karlkfi
test: close response body in watch tests
f99076c 
- Close response body after http calls in the watch tests
- Add apitesting utility methods for closing and testing errors
  from response body and websocket.
- Validate read after close errors.
@karlkfi karlkfi force-pushed the karl-fake-watcher-wait branch from c629d9c to f99076c Compare May 9, 2025 18:35
@k8s-ci-robot
Copy link
Contributor

@karlkfi: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kubernetes-e2e-kind f99076c link true /test pull-kubernetes-e2e-kind

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.

@karlkfi karlkfi changed the title refactor: watch test cleanup [WIP] refactor: watch test cleanup May 9, 2025
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 9, 2025
@k8s-ci-robot
Copy link
Contributor

PR needs rebase.

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.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deads2k deads2k Awaiting requested review from deads2k

@jpbetz jpbetz Awaiting requested review from jpbetz

Assignees

@wojtek-t wojtek-t

Labels
area/apiserver cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/flake Categorizes issue or PR as related to a flaky test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@karlkfi @k8s-ci-robot @siyuanfoundation @yongruilin @wojtek-t
Morty Proxy This is a proxified and sanitized view of the page, visit original site.