remove assert/require lib from scheduler pkg #131145

mohitsethia · 2025-04-01T18:34:09Z

What type of PR is this?

/sig scheduling
/kind cleanup

What this PR does / why we need it: According to the sig guidelines, we should use cmp.Equal or cmp.Diff instead of assert & require libraries

Which issue(s) this PR fixes: #130407

Fixes #130407

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2025-04-01T18:34:12Z

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.

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 · 2025-04-01T18:34:17Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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 · 2025-04-01T18:34:18Z

Welcome @mohitsethia!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2025-04-01T18:34:19Z

Hi @mohitsethia. Thanks for your PR.

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

googs1025 · 2025-04-02T00:53:32Z

Can you follow the original author's comment?
base on:
#130407 (comment)

or You don't need to use a new pr, but can continue to complete the work directly on that pr.

googs1025 · 2025-04-02T00:56:50Z

or you can use git commit --author xxxx to name the co-author

googs1025 · 2025-04-02T00:57:07Z

Can you follow the original author's comment? base on: #130407 (comment)

or You don't need to use a new pr, but can continue to complete the work directly on that pr.

/hold

mohitsethia · 2025-04-02T06:09:49Z

Hi @googs1025 , I had forked the repo with only the master branch. So, it was not possible for me to use that branch.

googs1025 · 2025-04-02T07:23:43Z

I had forked the repo with only the master branch. So, it was not possible for me to use that branch.

can you pull his branch down without recreating?

mohitsethia · 2025-04-02T07:59:19Z

Since this is already created, does it make sense to simply close that PR? And to pull his branch, I again have to clone his forked repo. I am sorry but I think this would only ~~complicate~~ (bad choice of word) and since the PR is already created, and I have already worked on this one, more changes than the previous PR.

googs1025 · 2025-04-02T08:33:29Z

@mohitsethia
Why do you need to fork his repository? Maybe you just need to use git remote add xxx 🤔

Since this is already created, does it make sense to simply close that PR?

base on:
#130407 (comment)
Maybe we should respect previous committers?

Or you need to indicate the collaborators in your PR. like this: #131145 (comment)

mohitsethia · 2025-04-10T21:39:17Z

/test pull-kubernetes-e2e-gce

macsko · 2025-04-07T08:53:09Z

pkg/scheduler/framework/plugins/dynamicresources/dynamicresources_test.go

@@ -1163,7 +1172,9 @@ func (tc *testContext) verify(t *testing.T, expected result, initialObjects []me
 func (tc *testContext) listAll(t *testing.T) (objects []metav1.Object) {
 	t.Helper()
 	claims, err := tc.client.ResourceV1beta1().ResourceClaims("").List(tc.ctx, metav1.ListOptions{})
-	require.NoError(t, err, "list claims")
+	if err != nil {
+		t.Errorf("unexpected error in list claims: %s", err.Error())


You don't need to call .Error() if passing is as an argument to the t.Errorf:

Suggested change

t.Errorf("unexpected error in list claims: %s", err.Error())

t.Errorf("unexpected error in list claims: %s", err)

macsko · 2025-04-07T08:55:20Z

pkg/scheduler/framework/plugins/nodeaffinity/node_affinity_test.go

+			if err != nil {
+				t.Errorf("Unexpected error: %s", err.Error())
+			}
+			if diff := cmp.Diff(tc.expectedHint, actualHint); diff != "" {


Using cmp.Diff on an int value is probably too much and just an equality check could be enough.

macsko · 2025-04-07T08:57:47Z

pkg/scheduler/util/queue/fifo_test.go

 }

 func verifyEmpty(t *testing.T, queue *FIFO[int]) {
 	t.Helper()
-	require.Equal(t, 0, queue.Len())
+	if !cmp.Equal(0, queue.Len()) {


For simple equality check it's not needed to use cmp.Equal:

Suggested change

if !cmp.Equal(0, queue.Len()) {

if queue.Len() != 0 {

I don't know why I didn't receive any notifications for these comments. The comment totally makes sense, fixed all the comments now.

mohitsethia · 2025-04-11T12:41:22Z

Hi , does any one know what seems to be the error in the e2e-gce test? Could it be a flaky test?

macsko · 2025-04-11T12:42:25Z

Hi , does any one know what seems to be the error in the e2e-gce test? Could it be a flaky test?

Looks like a flaky test

/retest

dgrisonnet · 2025-04-17T16:36:57Z

/remove-sig instrumentation

macsko · 2025-04-18T07:32:56Z

pkg/scheduler/util/queue/fifo_test.go

@@ -99,19 +111,31 @@ func TestShrink(t *testing.T) {
 	for i := 0; i < normalSize*2; i++ {
 		queue.Push(i)
 	}
-	require.Equal(t, normalSize*2, queue.Len())
-	require.LessOrEqual(t, 2*normalSize, len(queue.elements))
+	if !cmp.Equal(normalSize*2, queue.Len()) {


Suggested change

if !cmp.Equal(normalSize*2, queue.Len()) {

if queue.Len() != normalSize*2 {

macsko · 2025-04-18T07:33:16Z

pkg/scheduler/util/queue/fifo_test.go

+	if !cmp.Equal(normalSize*2, queue.Len()) {
+		t.Errorf("Expected queue len as %d, actual len: %d", normalSize*2, queue.Len())
+	}
+	if 2*normalSize > len(queue.elements) {


Flip the args:

Suggested change

if 2*normalSize > len(queue.elements) {

if len(queue.elements) < 2*normalSize {

macsko · 2025-04-18T07:33:32Z

pkg/scheduler/util/queue/fifo_test.go

+	if queue.Len() != 0 {
+		t.Errorf("Expected queue len as 0, actual len: %d", queue.Len())
+	}
+	if !cmp.Equal(normalSize, len(queue.elements)) {


Suggested change

if !cmp.Equal(normalSize, len(queue.elements)) {

if len(queue.elements) != normalSize {

macsko · 2025-04-18T07:33:48Z

pkg/scheduler/util/queue/fifo_test.go

+	if queue.Len() != 0 {
+		t.Errorf("Expected queue len as 0, actual len: %d", queue.Len())
+	}
+	if normalSize != len(queue.elements) {


Suggested change

if normalSize != len(queue.elements) {

if len(queue.elements) != normalSize {

macsko · 2025-04-18T07:34:33Z

pkg/scheduler/util/queue/fifo_test.go

+	if !cmp.Equal(expectedOk, ok) {
+		t.Errorf("Expected queue.Pop() status: %t, actual status: %t", expectedOk, ok)
+	}
+	if !cmp.Equal(expectedValue, actual) {
+		t.Errorf("Expected queue.Pop() value: %d, actual status: %d", expectedValue, actual)
+	}


Suggested change

if !cmp.Equal(expectedOk, ok) {

t.Errorf("Expected queue.Pop() status: %t, actual status: %t", expectedOk, ok)

}

if !cmp.Equal(expectedValue, actual) {

t.Errorf("Expected queue.Pop() value: %d, actual status: %d", expectedValue, actual)

}

if ok != expectedOk {

t.Errorf("Expected queue.Pop() status: %t, actual status: %t", expectedOk, ok)

}

if actual != expectedValue {

t.Errorf("Expected queue.Pop() value: %d, actual status: %d", expectedValue, actual)

}

Shall I resolve all the conversation? Or wait for the commentor to do it?

macsko · 2025-04-18T07:36:21Z

pkg/scheduler/framework/plugins/dynamicresources/dynamicresources_test.go

+	if expected.status == nil && status != nil {
+		t.Errorf("Expected nil status but got: %+v", status)


Maybe keep the previous logic:

Suggested change

if expected.status == nil && status != nil {

t.Errorf("Expected nil status but got: %+v", status)

if expected.status == nil {

if status != nil {

t.Errorf("Expected nil status but got: %+v", status)

}

I got a comment to simplify it 😅

BenTheElder · 2025-04-25T17:12:43Z

/remove-area dependency
needs rebase

mohitsethia · 2025-04-26T21:37:29Z

/test pull-kubernetes-unit

mohitsethia · 2025-04-27T20:33:43Z

Hi, can we merge the PR now, or is there anything that is blocking it for now?

mohitsethia · 2025-04-30T10:13:03Z

Hi @macsko, can you please help me letting know what is the next action to be taken for this PR?

neolit123

/remove-area kubeadm
/remove-sig cluster-lifecycle

k8s-ci-robot · 2025-06-07T13:56:31Z

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.

macsko

Please rebase and check if after rebasing no new assert/require usages appear.

macsko · 2025-06-30T08:27:36Z

pkg/scheduler/framework/plugins/dynamicresources/dynamicresources_test.go

@@ -1163,7 +1174,9 @@ func (tc *testContext) verify(t *testing.T, expected result, initialObjects []me
 func (tc *testContext) listAll(t *testing.T) (objects []metav1.Object) {
 	t.Helper()
 	claims, err := tc.client.ResourceV1beta1().ResourceClaims("").List(tc.ctx, metav1.ListOptions{})
-	require.NoError(t, err, "list claims")
+	if err != nil {
+		t.Errorf("unexpected error in list claims: %s", err)


Change to t.Fatalf everywhere where require was used

macsko · 2025-06-30T08:31:01Z

pkg/scheduler/framework/plugins/nodeports/node_ports_test.go

+			if err != nil {
+				t.Errorf("Unexpected error in isSchedulableAfterPodDeleted: %s", err.Error())
+			}
+			if diff := cmp.Diff(tc.expectedHint, actualHint); diff != "" {


It's an integer. You could just do comparison (tc.expectedHint != actualHint). It would be good to print them in error message as strings (use %s).

And change it similarly for other plugins

macsko · 2025-06-30T08:31:56Z

pkg/scheduler/framework/plugins/noderesources/fit_test.go

-			require.NoError(t, err)
-			require.Equal(t, tc.expectedHint, actualHint)
+			if err != nil {
+				t.Errorf("Unexpected error in isSchedulableAfterNodeChange: %s", err.Error())


You can skip .Error() here. Please change it in other places as well:

Suggested change

t.Errorf("Unexpected error in isSchedulableAfterNodeChange: %s", err.Error())

t.Errorf("Unexpected error in isSchedulableAfterNodeChange: %s", err)

macsko · 2025-06-30T08:32:37Z

pkg/scheduler/util/queue/fifo_test.go

+		t.Errorf("Expected queue.Pop() status: %t, actual status: %t", expectedOk, ok)
+	}
+	if actual != expectedValue {
+		t.Errorf("Expected queue.Pop() value: %d, actual status: %d", expectedValue, actual)


Suggested change

t.Errorf("Expected queue.Pop() value: %d, actual status: %d", expectedValue, actual)

t.Errorf("Expected queue.Pop() value: %d, actual value: %d", expectedValue, actual)

macsko · 2025-06-30T08:32:46Z

pkg/scheduler/util/queue/fifo_test.go

 }

 func verifyEmpty(t *testing.T, queue *FIFO[int]) {
 	t.Helper()
-	require.Equal(t, 0, queue.Len())
+	if queue.Len() != 0 {
+		t.Errorf("Expected empty queue, actual Len: %d", queue.Len())


Suggested change

t.Errorf("Expected empty queue, actual Len: %d", queue.Len())

t.Errorf("Expected empty queue, actual len: %d", queue.Len())

k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 1, 2025

k8s-ci-robot requested review from bart0sh and jingxu97 April 1, 2025 18:34

k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. sig/storage Categorizes an issue or PR as relevant to SIG Storage. wg/device-management Categorizes an issue or PR as relevant to WG Device Management. labels Apr 1, 2025

github-project-automation bot added this to SIG Node: code and documentation PRs Apr 1, 2025

github-project-automation bot moved this to Triage in SIG Node: code and documentation PRs Apr 1, 2025

github-project-automation bot added this to Dynamic Resource Allocation Apr 1, 2025

github-project-automation bot moved this to 🆕 New in Dynamic Resource Allocation Apr 1, 2025

mohitsethia mentioned this pull request Apr 1, 2025

Remove usage of assertion libraries in pkg/scheduler tests #130407

Open

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 2, 2025

mohitsethia force-pushed the remove-assert-lib-in-scheduler-tests branch from 726b639 to 42be468 Compare April 10, 2025 20:29

macsko reviewed Apr 11, 2025

View reviewed changes

k8s-ci-robot removed the sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. label Apr 17, 2025

mohitsethia force-pushed the remove-assert-lib-in-scheduler-tests branch 2 times, most recently from fea491c to b4bb6dc Compare April 17, 2025 21:24

macsko reviewed Apr 18, 2025

View reviewed changes

mohitsethia force-pushed the remove-assert-lib-in-scheduler-tests branch from b4bb6dc to 6ad0083 Compare April 18, 2025 09:58

mohitsethia requested a review from macsko April 23, 2025 08:07

SergeyKanzhelev moved this from Triage to Archive-it in SIG Node CI/Test Board Apr 23, 2025

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 23, 2025

k8s-ci-robot removed the area/dependency Issues or PRs related to dependency changes label Apr 25, 2025

remove assert/require lib from scheduler pkg

09376ee

mohitsethia force-pushed the remove-assert-lib-in-scheduler-tests branch from 6ad0083 to 09376ee Compare April 26, 2025 21:02

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 26, 2025

neolit123 reviewed May 6, 2025

View reviewed changes

k8s-ci-robot removed area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. labels May 6, 2025

aramase moved this from Needs Triage to !SIG Auth in SIG Auth May 12, 2025

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 7, 2025

macsko reviewed Jun 30, 2025

View reviewed changes

macsko mentioned this pull request Jun 30, 2025

test: assert pkg removal in pkg/scheduler #132612

Closed

	t.Errorf("unexpected error in list claims: %s", err.Error())
	t.Errorf("unexpected error in list claims: %s", err)

	if !cmp.Equal(normalSize*2, queue.Len()) {
	if queue.Len() != normalSize*2 {

	if 2*normalSize > len(queue.elements) {
	if len(queue.elements) < 2*normalSize {

	if !cmp.Equal(normalSize, len(queue.elements)) {
	if len(queue.elements) != normalSize {

	if normalSize != len(queue.elements) {
	if len(queue.elements) != normalSize {

		if expected.status == nil && status != nil {
		t.Errorf("Expected nil status but got: %+v", status)

	t.Errorf("Unexpected error in isSchedulableAfterNodeChange: %s", err.Error())
	t.Errorf("Unexpected error in isSchedulableAfterNodeChange: %s", err)

	t.Errorf("Expected queue.Pop() value: %d, actual status: %d", expectedValue, actual)
	t.Errorf("Expected queue.Pop() value: %d, actual value: %d", expectedValue, actual)

	t.Errorf("Expected empty queue, actual Len: %d", queue.Len())
	t.Errorf("Expected empty queue, actual len: %d", queue.Len())

remove assert/require lib from scheduler pkg #131145

Are you sure you want to change the base?

remove assert/require lib from scheduler pkg #131145

Uh oh!

Conversation

mohitsethia commented Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

What this PR does / why we need it: According to the sig guidelines, we should use cmp.Equal or cmp.Diff instead of assert & require libraries

Which issue(s) this PR fixes: #130407

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Uh oh!

k8s-ci-robot commented Apr 1, 2025

Uh oh!

k8s-ci-robot commented Apr 1, 2025

Uh oh!

k8s-ci-robot commented Apr 1, 2025

Uh oh!

k8s-ci-robot commented Apr 1, 2025

Uh oh!

googs1025 commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googs1025 commented Apr 2, 2025

Uh oh!

googs1025 commented Apr 2, 2025

Uh oh!

mohitsethia commented Apr 2, 2025

Uh oh!

googs1025 commented Apr 2, 2025

Uh oh!

mohitsethia commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googs1025 commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mohitsethia commented Apr 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mohitsethia Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mohitsethia commented Apr 11, 2025

Uh oh!

macsko commented Apr 11, 2025

Uh oh!

dgrisonnet commented Apr 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenTheElder commented Apr 25, 2025

Uh oh!

mohitsethia commented Apr 26, 2025

Uh oh!

mohitsethia commented Apr 27, 2025

Uh oh!

mohitsethia commented Apr 30, 2025

mohitsethia commented Apr 1, 2025 •

edited

Loading

googs1025 commented Apr 2, 2025 •

edited

Loading

mohitsethia commented Apr 2, 2025 •

edited

Loading

googs1025 commented Apr 2, 2025 •

edited

Loading

mohitsethia Apr 17, 2025 •

edited

Loading