test: add unit tests for the priority package #228

shuheiktgw · 2022-04-21T10:36:41Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Added missing unit tests to the priority package (and fix a minor bug in it)

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

k8s-ci-robot · 2022-04-21T10:36:48Z

Welcome @shuheiktgw!

It looks like this is your first PR to kubernetes-sigs/kueue 🎉. 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-sigs/kueue 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 · 2022-04-21T10:36:49Z

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

shuheiktgw · 2022-04-21T10:38:00Z

pkg/util/priority/priority.go

@@ -77,6 +77,7 @@ func getDefaultPriorityClass(ctx context.Context, client client.Client) (*schedu
 	// we pick the one with the lowest priority value.
 	var defaultPC *schedulingv1.PriorityClass
 	for _, pci := range pcs.Items {
+		pci := pci


This is a pretty common mistake we sometimes see. You can test why it is a problem with the "priorityClass is unspecified and multiple global default exist" test case 🙂

I still can't understand this. Can you add more details or informations for it?

The article below seems to explain the problem in detail. In short, the variable pci is reused for each iteration, so referring to the pointer of pci ends up pointing to the last element of pcs.Items🙂

https://medium.com/swlh/use-pointer-of-for-range-loop-variable-in-go-3d3481f7ffc9

pci := pci may be confusing so I renamed it

denkensk · 2022-04-21T10:38:18Z

/ok-to-test

shuheiktgw · 2022-04-21T10:38:39Z

pkg/util/priority/priority_test.go

+			expectedValue: 40,
+		},
+		{
+			desc: "priorityClass is unspecified and multiple global default exist",


Is it possible that priorityClass is unspecified and no global default exists?

Maybe. Then it will use constants.DefaultPriority

pkg/util/priority/priority_test.go

denkensk · 2022-04-21T10:44:27Z

pkg/util/priority/priority_test.go

+		expectedName      string
+		expectedValue     int32


Suggested change

expectedName string

expectedValue int32

expectedPriorityClassName string

expectedPriorityClassValue int32

denkensk · 2022-04-21T10:49:40Z

pkg/util/priority/priority_test.go

+	}{
+		{
+			desc: "priority is specified",
+			workload: &kueue.Workload{


use MakeWorkload in wrappers.go

denkensk · 2022-04-21T10:51:26Z

pkg/util/priority/priority_test.go

+			expectedValue: 40,
+		},
+		{
+			desc: "priorityClass is unspecified and multiple global default exist",


Maybe. Then it will use constants.DefaultPriority

denkensk · 2022-04-21T10:52:48Z

pkg/util/priority/priority.go

@@ -77,6 +77,7 @@ func getDefaultPriorityClass(ctx context.Context, client client.Client) (*schedu
 	// we pick the one with the lowest priority value.
 	var defaultPC *schedulingv1.PriorityClass
 	for _, pci := range pcs.Items {
+		pci := pci


I still can't understand this. Can you add more details or informations for it?

denkensk · 2022-04-21T10:54:41Z

pkg/util/priority/priority_test.go

+				Items: []schedulingv1.PriorityClass{},
+			},
+			priorityClassName: "test",
+			expectedErr:       true,


Can you also check the error message here?

shuheiktgw · 2022-04-22T00:29:01Z

@denkensk Fixed it! Would you review the PR again? 🙏

denkensk · 2022-04-22T03:19:36Z

Can you squash your commits to one？ LGTM overall

shuheiktgw · 2022-04-23T00:19:37Z

@denkensk Thanks! Squashed 🙏

denkensk · 2022-04-23T03:20:01Z

/lgtm

Thanks. @shuheiktgw

denkensk · 2022-04-23T03:20:25Z

ping @ahg-g for approve.

pkg/util/priority/priority_test.go

ahg-g

thanks, a couple of nits

ahg-g · 2022-04-25T03:51:40Z

pkg/util/priority/priority_test.go

+			name, value, err := GetPriorityFromPriorityClass(context.Background(), client, tt.priorityClassName)
+			if tt.expectedErr != "" {
+				if err == nil {
+					t.Fatalf("expected error but error is nil")


Suggested change

t.Fatalf("expected error but error is nil")

t.Fatalf("expected an error")

ahg-g · 2022-04-25T03:53:20Z

pkg/util/priority/priority_test.go

+				if !strings.Contains(err.Error(), tt.expectedErr) {
+					t.Fatalf("unexpecter error message: got: %s, expected: %s", err.Error(), tt.expectedErr)
+				}


Suggested change

if !strings.Contains(err.Error(), tt.expectedErr) {

t.Fatalf("unexpecter error message: got: %s, expected: %s", err.Error(), tt.expectedErr)

}

if diff := cmp.Diff(tt.expectedErr, err); diff != "" {

t.Errorf("unexpected error (-want,+got):\n%s", diff)

}

pkg/util/priority/priority_test.go

alculquicondor · 2022-04-25T15:00:06Z

pkg/util/priority/priority.go

@@ -77,9 +77,10 @@ func getDefaultPriorityClass(ctx context.Context, client client.Client) (*schedu
 	// we pick the one with the lowest priority value.
 	var defaultPC *schedulingv1.PriorityClass


Maybe keep a copy, instead of a pointer.

Also returning a copy instead of a pointer should be fine.

I'm not sure if I understand it correctly but you meant like the one below?

var defaultPC schedulingv1.PriorityClass var found bool for _, pci := range pcs.Items { if pci.GlobalDefault { if !found || defaultPC.Value > pci.Value { found = true defaultPC = pci } } } if !found { return nil, nil } return &defaultPC, nil

yes, but also just return defaultPC, nil.

What you have is fine too though.

I see! It currently checks if the default priority class is nil or not so returning schedulingv1.PriorityClass from the getDefaultPriorityClass may complicate the check a little. Let's stick with returning *schedulingv1.PriorityClass then 🙂

@alculquicondor I've updated it and squashed the commits. Would you review the PR again? 🙏

alculquicondor · 2022-04-29T13:48:35Z

pkg/util/priority/priority.go

@@ -75,13 +75,20 @@ func getDefaultPriorityClass(ctx context.Context, client client.Client) (*schedu

 	// In case more than one global default priority class is added as a result of a race condition,
 	// we pick the one with the lowest priority value.
-	var defaultPC *schedulingv1.PriorityClass
+	var defaultPC schedulingv1.PriorityClass


Nit: if you are still returning a pointer, I think the initial implementation was simpler, as it does not need a boolean.

alculquicondor · 2022-04-29T13:50:31Z

pkg/util/priority/priority_test.go

+		t.Fatalf("Failed adding scheduling scheme: %v", err)
+	}
+
+	tests := []struct {


Nit, just to be consistent with other tests: use map[string]struct{}

alculquicondor · 2022-04-29T13:51:51Z

pkg/util/priority/priority_test.go

+		expectedPriorityClassName  string
+		expectedPriorityClassValue int32
+		expectedErr                string


Suggested change

expectedPriorityClassName string

expectedPriorityClassValue int32

expectedErr string

wantPriorityClassName string

wantPriorityClassValue int32

wantErr string

shuheiktgw · 2022-05-11T00:06:03Z

/reopen

k8s-ci-robot · 2022-05-11T00:06:14Z

@shuheiktgw: Reopened this PR.

In response to this:

/reopen

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.

shuheiktgw · 2022-05-11T00:32:53Z

@alculquicondor Sorry that I missed Prow somehow closed the PR! I've updated the PR again so would you review it? 🙇 (I'll squash the commits again once it's been approved)

alculquicondor

Thanks,
please squash

/approve

k8s-ci-robot · 2022-05-11T20:21:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, shuheiktgw

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~~ [alculquicondor]

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

shuheiktgw · 2022-05-12T11:01:54Z

Thanks, I squashed the commits 👍

alculquicondor · 2022-05-12T13:24:34Z

/lgtm

k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 21, 2022

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 21, 2022

k8s-ci-robot requested review from ArangoGutierrez and denkensk April 21, 2022 10:36

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 21, 2022

shuheiktgw commented Apr 21, 2022

View reviewed changes

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 Apr 21, 2022

shuheiktgw commented Apr 21, 2022

View reviewed changes

denkensk reviewed Apr 21, 2022

View reviewed changes

shuheiktgw force-pushed the add_priority_testcases branch from 396660d to 0acb185 Compare April 23, 2022 00:18

shuheiktgw mentioned this pull request Apr 23, 2022

Enable goimports #229

Merged

k8s-ci-robot assigned denkensk Apr 23, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 23, 2022

kerthcet reviewed Apr 24, 2022

View reviewed changes

pkg/util/priority/priority_test.go Outdated Show resolved Hide resolved

ahg-g reviewed Apr 25, 2022

View reviewed changes

alculquicondor reviewed Apr 25, 2022

View reviewed changes

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 26, 2022

shuheiktgw force-pushed the add_priority_testcases branch from 3996013 to 46e50e7 Compare April 29, 2022 01:40

alculquicondor reviewed Apr 29, 2022

View reviewed changes

k8s-ci-robot closed this in #229 Apr 29, 2022

k8s-ci-robot reopened this May 11, 2022

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

shuheiktgw force-pushed the add_priority_testcases branch from 46e50e7 to af08e93 Compare May 11, 2022 00:09

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

alculquicondor reviewed May 11, 2022

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 11, 2022

Add unit tests for the priority package

41de246

shuheiktgw force-pushed the add_priority_testcases branch from c095746 to 41de246 Compare May 12, 2022 11:01

k8s-ci-robot assigned alculquicondor May 12, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 12, 2022

k8s-ci-robot merged commit 68ae767 into kubernetes-sigs:main May 12, 2022

	t.Fatalf("expected error but error is nil")
	t.Fatalf("expected an error")

		@@ -77,9 +77,10 @@ func getDefaultPriorityClass(ctx context.Context, client client.Client) (*schedu
		// we pick the one with the lowest priority value.
		var defaultPC *schedulingv1.PriorityClass

test: add unit tests for the priority package #228

test: add unit tests for the priority package #228

Conversation

shuheiktgw commented Apr 21, 2022

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:

k8s-ci-robot commented Apr 21, 2022

k8s-ci-robot commented Apr 21, 2022

shuheiktgw Apr 21, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denkensk commented Apr 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shuheiktgw commented Apr 22, 2022

denkensk commented Apr 22, 2022 • edited

shuheiktgw commented Apr 23, 2022

denkensk commented Apr 23, 2022

denkensk commented Apr 23, 2022

ahg-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shuheiktgw commented May 11, 2022

k8s-ci-robot commented May 11, 2022

shuheiktgw commented May 11, 2022 • edited

alculquicondor left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 11, 2022

shuheiktgw commented May 12, 2022

alculquicondor commented May 12, 2022

shuheiktgw Apr 21, 2022 •

edited

denkensk commented Apr 22, 2022 •

edited

shuheiktgw commented May 11, 2022 •

edited