Use testing.TB.Context() instead of context.Background() #4754

Horiodino · 2025-03-22T14:15:07Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Go 1.24, testing.TB.Context(t) is supported, allowing us to replace all instances of context.Background() in tests

Which issue(s) this PR fixes:

Fixes #4655 (comment)

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

k8s-ci-robot · 2025-03-22T14:15:17Z

Hi @Horiodino. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

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.

netlify · 2025-03-22T14:15:25Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`69dc204`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/67e02ded3b655c0008486c61

mbobrovskyi · 2025-03-22T15:01:28Z

/ok-to-test

cmd/importer/pod/check_test.go

pkg/controller/admissionchecks/multikueue/indexer_test.go

pkg/controller/admissionchecks/multikueue/multikueuecluster_test.go

pkg/controller/admissionchecks/multikueue/indexer_test.go

mbobrovskyi

Can we update it here as well?

kueue/pkg/util/testing/context.go

Line 32 in 76352d7

return ctrl.LoggerInto(context.Background(), logger), logger

mbobrovskyi

For follow up we need to update integration and e2e tests as well.

Horiodino · 2025-03-23T06:56:29Z

For follow up we need to update integration and e2e tests as well.

E2E test? It's using Ginkgo

mbobrovskyi · 2025-03-23T07:02:31Z

For follow up we need to update integration and e2e tests as well.

E2E test? It's using Ginkgo

Yeah. I think we can get context from ginkgo.GinkgoTB().Context().

Horiodino · 2025-03-23T07:06:12Z

Oh, I thought you were referring to using t.Context(). My bad!

mbobrovskyi · 2025-03-23T07:17:19Z

Oh, I thought you were referring to using t.Context(). My bad!

Yeah ginkgo.GinkgoTB() is a wrapper for testing.TB().

Signed-off-by: Horiodino <[email protected]> Using ctx.Background() cant parse nil. Signed-off-by: Horiodino <[email protected]> Remove unused context return value from getClientBuilder function Signed-off-by: Horiodino <[email protected]> Use ginkgo.GinkgoTB().Context() instead of context.Background() Signed-off-by: Horiodino <[email protected]>

mbobrovskyi · 2025-03-23T13:17:22Z

@Horiodino try upgrading Ginkgo to v2.23.3. It looks like they fixed this bug.

test/performance/scheduler/minimalkueue/main.go

mbobrovskyi · 2025-03-23T13:35:27Z

test/performance/scheduler/runner/main.go

@@ -103,7 +105,7 @@ func main() {
 	log.Info("Start runner", "outputDir", outputDir, "crdsPath", crdsPath)
 	errCh := make(chan error, 3)
 	wg := &sync.WaitGroup{}
-	ctx, ctxCancel := context.WithCancel(ctrl.LoggerInto(context.Background(), log))
+	ctx, ctxCancel := context.WithCancel(ctrl.LoggerInto(ginkgo.GinkgoT().Context(), log))


Horiodino · 2025-03-23T14:20:24Z

@Horiodino try upgrading Ginkgo to v2.23.3. It looks like they fixed this bug.

GinkgoTB().Context() is picking up a different instance of Context()—one that’s valid at compile time but panics at runtime due to the interface embedding. if you run ctx = ginkgo.GinkgoT().Context() it will run .


  [PANICKED] Test Panicked
  In [BeforeSuite] at: /usr/lib/go/src/runtime/panic.go:262 @ 03/23/25 19:21:24.159

  runtime error: invalid memory address or nil pointer dereference

  Full Stack Trace
    sigs.k8s.io/kueue/test/integration/framework.(*Framework).SetupClient(0xc00030e0e0, 0xc000218908)
    	/home/yoi/miku/kueue-ctx/test/integration/framework/framework.go:136 +0x7d7
    sigs.k8s.io/kueue/test/integration/singlecluster/controller/admissionchecks/provisioning.init.func2()
    	/home/yoi/miku/kueue-ctx/test/integration/singlecluster/controller/admissionchecks/provisioning/suite_test.go:63 +0x26e
------------------------------
[BeforeSuite] [PANICKED] [10.206 seconds]
[BeforeSuite]

ginkgo version
Ginkgo Version 2.23.3

Signed-off-by: Horiodino <[email protected]>

mbobrovskyi · 2025-03-23T16:07:35Z

GinkgoTB().Context() is picking up a different instance of Context()—one that’s valid at compile time but panics at runtime due to the interface embedding. if you run ctx = ginkgo.GinkgoT().Context() it will run .

It's very strange - locally, v2.23.3 works fine with ginkgo.GinkgoTB(). But okay, ginkgo.GinkgoT() is good.

mbobrovskyi

/lgtm

Awesome! Thank you so much!

k8s-ci-robot · 2025-03-23T16:09:40Z

LGTM label has been added.

Git tree hash: 3e5c2c38a5c675b3ce77eb4e34d08230eda9c675

mimowo · 2025-03-24T06:41:06Z

/approve
Thanks!

k8s-ci-robot · 2025-03-24T06:41:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Horiodino, mbobrovskyi, mimowo

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mimowo]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added release-note-none kind/cleanup labels Mar 22, 2025

k8s-ci-robot requested review from kannon92 and tenzen-y March 22, 2025 14:15

k8s-ci-robot added cncf-cla: yes needs-ok-to-test labels Mar 22, 2025

k8s-ci-robot added the size/L label Mar 22, 2025

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Mar 22, 2025

mbobrovskyi reviewed Mar 22, 2025

View reviewed changes

cmd/importer/pod/check_test.go Outdated Show resolved Hide resolved

mbobrovskyi reviewed Mar 22, 2025

View reviewed changes

pkg/controller/admissionchecks/multikueue/indexer_test.go Outdated Show resolved Hide resolved

Horiodino force-pushed the t-ctx branch 3 times, most recently from 9be1959 to d7cd901 Compare March 22, 2025 16:29

Horiodino commented Mar 22, 2025

View reviewed changes

pkg/controller/admissionchecks/multikueue/multikueuecluster_test.go Outdated Show resolved Hide resolved

mbobrovskyi reviewed Mar 23, 2025

View reviewed changes

pkg/controller/admissionchecks/multikueue/indexer_test.go Outdated Show resolved Hide resolved

Horiodino force-pushed the t-ctx branch from 499d47c to 76352d7 Compare March 23, 2025 05:25

mbobrovskyi reviewed Mar 23, 2025

View reviewed changes

Horiodino force-pushed the t-ctx branch from 76352d7 to d11ad60 Compare March 23, 2025 11:46

Horiodino marked this pull request as draft March 23, 2025 12:11

k8s-ci-robot added the do-not-merge/work-in-progress label Mar 23, 2025

Horiodino marked this pull request as ready for review March 23, 2025 12:55

k8s-ci-robot removed the do-not-merge/work-in-progress label Mar 23, 2025

k8s-ci-robot requested a review from mimowo March 23, 2025 12:55

mbobrovskyi reviewed Mar 23, 2025

View reviewed changes

test/performance/scheduler/minimalkueue/main.go Outdated Show resolved Hide resolved

mbobrovskyi reviewed Mar 23, 2025

View reviewed changes

Horiodino force-pushed the t-ctx branch from 7ad09d1 to 69dc204 Compare March 23, 2025 15:51

mbobrovskyi approved these changes Mar 23, 2025

View reviewed changes

k8s-ci-robot assigned mbobrovskyi Mar 23, 2025

k8s-ci-robot added the lgtm label Mar 23, 2025

k8s-ci-robot added the approved label Mar 24, 2025

k8s-ci-robot merged commit e8c167e into kubernetes-sigs:main Mar 24, 2025
19 of 21 checks passed

k8s-ci-robot added this to the v0.12 milestone Mar 24, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use testing.TB.Context() instead of context.Background() #4754

Use testing.TB.Context() instead of context.Background() #4754

Horiodino commented Mar 22, 2025

k8s-ci-robot commented Mar 22, 2025

netlify bot commented Mar 22, 2025 •

edited

Loading

mbobrovskyi commented Mar 22, 2025

mbobrovskyi left a comment

mbobrovskyi left a comment

Horiodino commented Mar 23, 2025

mbobrovskyi commented Mar 23, 2025

Horiodino commented Mar 23, 2025

mbobrovskyi commented Mar 23, 2025

mbobrovskyi commented Mar 23, 2025 •

edited

Loading

mbobrovskyi Mar 23, 2025

Horiodino commented Mar 23, 2025 •

edited

Loading

mbobrovskyi commented Mar 23, 2025

mbobrovskyi left a comment

k8s-ci-robot commented Mar 23, 2025

mimowo commented Mar 24, 2025

k8s-ci-robot commented Mar 24, 2025

Use testing.TB.Context() instead of context.Background() #4754

Use testing.TB.Context() instead of context.Background() #4754

Conversation

Horiodino commented Mar 22, 2025

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

k8s-ci-robot commented Mar 22, 2025

netlify bot commented Mar 22, 2025 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

mbobrovskyi commented Mar 22, 2025

mbobrovskyi left a comment

Choose a reason for hiding this comment

mbobrovskyi left a comment

Choose a reason for hiding this comment

Horiodino commented Mar 23, 2025

mbobrovskyi commented Mar 23, 2025

Horiodino commented Mar 23, 2025

mbobrovskyi commented Mar 23, 2025

mbobrovskyi commented Mar 23, 2025 • edited Loading

mbobrovskyi Mar 23, 2025

Choose a reason for hiding this comment

Horiodino commented Mar 23, 2025 • edited Loading

mbobrovskyi commented Mar 23, 2025

mbobrovskyi left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 23, 2025

mimowo commented Mar 24, 2025

k8s-ci-robot commented Mar 24, 2025

netlify bot commented Mar 22, 2025 •

edited

Loading

mbobrovskyi commented Mar 23, 2025 •

edited

Loading

Horiodino commented Mar 23, 2025 •

edited

Loading