Skip to content

Conversation

@majiayu000
Copy link

Fixes #3535

Changes

  • Replace ctx, cancel := context.WithCancel(context.Background()) pattern with t.Context() in test files
  • Remove unused defer cancel() / t.Cleanup(cancel) calls
  • Remove unused context imports where applicable

This is a mechanical refactoring that uses Go 1.21's testing.T.Context() method which automatically cancels the context when the test completes.

Replace manual context.WithCancel(context.Background()) pattern with
t.Context() in test files. This is a mechanical refactoring that uses
Go 1.21's testing.T.Context() method which automatically cancels the
context when the test completes.

Fixes kcp-dev#3535

Signed-off-by: majiayu000 <[email protected]>
@kcp-ci-bot
Copy link
Contributor

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Details

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/test-infra repository.

@kcp-ci-bot kcp-ci-bot added dco-signoff: yes Indicates the PR's author has signed the DCO. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jan 1, 2026
@kcp-ci-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign ntnn for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details 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

@kcp-ci-bot kcp-ci-bot added do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 1, 2026
@kcp-ci-bot
Copy link
Contributor

Hi @majiayu000. Thanks for your PR.

I'm waiting for a kcp-dev member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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/test-infra repository.

@mjudeikis
Copy link
Contributor

/ok-to-test
@ntnn feels like I see this one already. Do you remember why we didnt refactor contexts before?

@kcp-ci-bot kcp-ci-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 2, 2026
@mjudeikis
Copy link
Contributor

ah, @xrstf left comment about this #3535 (comment)
lets see what CI will tell us on few runs

@kcp-ci-bot
Copy link
Contributor

@majiayu000: The following tests 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-kcp-verify-codegen 59f9ef4 link true /test pull-kcp-verify-codegen
pull-kcp-test-e2e-sharded 59f9ef4 link true /test pull-kcp-test-e2e-sharded

Full PR test history

Details

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/test-infra repository. I understand the commands that are listed here.

@ntnn
Copy link
Member

ntnn commented Jan 2, 2026

a) What @xrstf said; we can't replace it everywhere, only where cancelling isn't needed.
b) Same qualms I've had with the other PR: #3583 (comment)
Just replacing the ctx := context.WithCancel(context.Background()) with a ctx := t.Context() isn't making the code any cleaner.
In the cases where cancelling isn't needed the assignment to ctx should removed altogether and t.Context() be used in the calls.

@mjudeikis
Copy link
Contributor

@majiayu000 how confident you are that your pr adresses above comments ^ or was it just AI generated replace all?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dco-signoff: yes Indicates the PR's author has signed the DCO. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update to use testing.T.Context

4 participants