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

pjsharath28 · 2025-03-11T05:48:11Z

What type of PR is this?

/kind cleanup
/sig scheduling

What this PR does / why we need it:

Removed the usage of assertion libraries as per the style guide

Which issue(s) this PR fixes:

Fixes #130407

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

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

k8s-ci-robot · 2025-03-11T05:48:20Z

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-03-11T05:48:21Z

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

k8s-ci-robot · 2025-03-11T05:48:25Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: pjsharath28
Once this PR has been reviewed and has the lgtm label, please assign alculquicondor, x13n for approval. 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

haosdent · 2025-03-12T08:02:38Z

/ok-to-test

haosdent · 2025-03-12T08:03:28Z

Actually, I feel using assert.InDelta looks more simple compared to the original code. But just my opinion

k8s-ci-robot · 2025-03-12T08:34:18Z

@pjsharath28: 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-unit	`939d617`	link	true	`/test pull-kubernetes-unit`

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.

macsko

Please rebase for the recent changes and replace require library calls as well

macsko · 2025-03-24T10:34:59Z

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

@@ -936,7 +936,9 @@ func TestPlugin(t *testing.T) {

 			result, status := testCtx.p.PreFilter(testCtx.ctx, testCtx.state, tc.pod)
 			t.Run("prefilter", func(t *testing.T) {
-				assert.Equal(t, tc.want.preFilterResult, result)
+				if diff := cmp.Diff(tc.want.preFilterResult, result); diff != "" {
+					t.Errorf("Mismatch in Prefilter (- expected, + actual):\n%s", diff)


Suggested change

t.Errorf("Mismatch in Prefilter (- expected, + actual):\n%s", diff)

t.Errorf("Unexpected PreFilter result (-want,+got):\n%s", diff)

And change in other as well

macsko · 2025-03-24T10:37:05Z

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

+		expectedErr := expected.status.AsError()
+		if expectedErr != nil {
+			if diff := cmp.Diff(expectedErr.Error(), actualErr.Error()); diff != "" {
+				t.Errorf("Mismatch in error strings (- expected, + actual):\n%s", diff)
+			}
+		} else {
+			t.Errorf("Expected no error, but got: %v", actualErr)
+		}


Use cmp.Equal to compare strings:

Suggested change

expectedErr := expected.status.AsError()

if expectedErr != nil {

if diff := cmp.Diff(expectedErr.Error(), actualErr.Error()); diff != "" {

t.Errorf("Mismatch in error strings (- expected, + actual):\n%s", diff)

}

} else {

t.Errorf("Expected no error, but got: %v", actualErr)

}

expectedErr := expected.status.AsError()

if !cmp.Equal(expectedErr.Error(), actualErr.Error()) {

t.Errorf("Expected status error: %s, got: %s", expectedErr, actualErr)

}

macsko · 2025-03-24T10:38:06Z

pkg/scheduler/framework/plugins/volumebinding/volume_binding_test.go

@@ -809,7 +812,9 @@ func TestVolumeBinding(t *testing.T) {
 			t.Logf("Verify: call Filter and check status")
 			for i, nodeInfo := range nodeInfos {
 				gotStatus := p.Filter(ctx, state, item.pod, nodeInfo)
-				assert.Equal(t, item.wantFilterStatus[i], gotStatus)
+				if diff := cmp.Diff(item.wantFilterStatus[i], gotStatus); diff != "" {
+					t.Errorf("Mismatch in wantFilterStatus[%d] (-want,+got):\n%s", i, diff)


Suggested change

t.Errorf("Mismatch in wantFilterStatus[%d] (-want,+got):\n%s", i, diff)

t.Errorf("Unexpected Filter status for node %s (-want,+got):\n%s", nodeInfo.Node().Name, diff)

macsko · 2025-03-24T10:46:01Z

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

+				actual := function(assertion.p)
+				delta := 0.1
+				expectedAsFloat := float64(assertion.expected)
+				actualAsFloat := float64(actual)
+				if (expectedAsFloat-actualAsFloat) > delta || (actualAsFloat-expectedAsFloat) > delta {
+					t.Errorf("Mismatch in values with delta threshold (%.2f): expected %.2f, actual %.2f", delta, expectedAsFloat, actualAsFloat)
+				}


Checking delta in this test doesn't make sense at all. We have two ints, so if they are equal the test will pass, but it they are not, it will always fail. It could be just an equality check here:

Suggested change

actual := function(assertion.p)

delta := 0.1

expectedAsFloat := float64(assertion.expected)

actualAsFloat := float64(actual)

if (expectedAsFloat-actualAsFloat) > delta || (actualAsFloat-expectedAsFloat) > delta {

t.Errorf("Mismatch in values with delta threshold (%.2f): expected %.2f, actual %.2f", delta, expectedAsFloat, actualAsFloat)

}

if actual := function(assertion.p); actual != assertion.expected {

t.Errorf(...)

}

macsko · 2025-03-24T11:05:33Z

pkg/scheduler/framework/autoscaler_contract/framework_contract_test.go

+	if !cmp.Equal(c, nil) {
+		t.Errorf("Expected nil, but got: %+v", c)
+	}
+


It seems like the nil check is redundant, you could use underscore above:

var _ frameworkContract = f

Code won't compile if the contract is not satisfied

Probably these two lines could be moved outside of the function, as package variables

k8s-ci-robot · 2025-04-25T08:42:26Z

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-triage-robot · 2025-08-11T11:57:49Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle rotten
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

macsko · 2025-08-11T11:59:48Z

/remove-lifecycle rotten

Remove usage of assertion libraries in pkg/scheduler tests

939d617

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 Mar 11, 2025

k8s-ci-robot requested review from gjtempleton and klueska March 11, 2025 05:48

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 Mar 11, 2025

github-project-automation bot added this to SIG Node: code and documentation PRs and Dynamic Resource Allocation Mar 11, 2025

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

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

pjsharath28 mentioned this pull request Mar 11, 2025

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

Open

k8s-ci-robot 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 Mar 12, 2025

bart0sh moved this from Triage to not-only-sig-node in SIG Node: code and documentation PRs Mar 19, 2025

macsko reviewed Mar 24, 2025

View reviewed changes

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

dims added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 12, 2025

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Aug 11, 2025

k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Aug 11, 2025

	t.Errorf("Mismatch in Prefilter (- expected, + actual):\n%s", diff)
	t.Errorf("Unexpected PreFilter result (-want,+got):\n%s", diff)

	t.Errorf("Mismatch in wantFilterStatus[%d] (-want,+got):\n%s", i, diff)
	t.Errorf("Unexpected Filter status for node %s (-want,+got):\n%s", nodeInfo.Node().Name, diff)

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

pjsharath28 commented Mar 11, 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?

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

Uh oh!

k8s-ci-robot commented Mar 11, 2025

Uh oh!

k8s-ci-robot commented Mar 11, 2025

Uh oh!

k8s-ci-robot commented Mar 11, 2025

Uh oh!

haosdent commented Mar 12, 2025

Uh oh!

haosdent commented Mar 12, 2025

Uh oh!

k8s-ci-robot commented Mar 12, 2025

Uh oh!

macsko left a comment

Choose a reason for hiding this comment

Uh oh!

macsko Mar 24, 2025

Choose a reason for hiding this comment

Uh oh!

macsko Mar 24, 2025

Choose a reason for hiding this comment

Uh oh!

macsko Mar 24, 2025

Choose a reason for hiding this comment

Uh oh!

macsko Mar 24, 2025

Choose a reason for hiding this comment

Uh oh!

macsko Mar 24, 2025

Choose a reason for hiding this comment

Uh oh!

macsko Mar 24, 2025

Choose a reason for hiding this comment

Uh oh!

macsko Mar 24, 2025

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Apr 25, 2025

Uh oh!

k8s-triage-robot commented Aug 11, 2025

Uh oh!

macsko commented Aug 11, 2025

Uh oh!

Uh oh!