use subtest for table units (pkg/scheduler/algorithmprovider) #63662

xchapter7x · 2018-05-10T11:26:49Z

What this PR does / why we need it: Update scheduler's unit table tests to use subtest

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

Special notes for your reviewer:
breaks up PR: #63281
/ref #63267

Release note:

This PR will leverage subtests on the existing table tests for the scheduler units.
Some refactoring of error/status messages and functions to align with new approach.

misterikkit · 2018-05-11T20:05:56Z

pkg/scheduler/algorithmprovider/plugins_test.go

-		for _, pf := range p.PriorityFunctionKeys.List() {
-			if !factory.IsPriorityFunctionRegistered(pf) {
-				t.Errorf("priority function %s is not registered but is used in the %s algorithm provider", pf, pn)
+		t.Run(fmt.Sprintf("providername/%s", pn), func(t *testing.T) {


"providername/" does not disambiguate anything here, but I think adding the "priority" and "predicate" names below is useful to disambiguate "sibling" subtests. I suggest removing "providername/"

misterikkit · 2018-05-11T20:08:28Z

pkg/scheduler/algorithmprovider/plugins_test.go

+			p, err := factory.GetAlgorithmProvider(pn)
+			if err != nil {
+				t.Errorf("error retrieving provider: %v", err)
+				return


t.Errorf();return can be replace with t.Fatalf()

misterikkit · 2018-05-11T20:10:44Z

pkg/scheduler/algorithmprovider/plugins_test.go

 			}
-		}
+			for _, pf := range p.PriorityFunctionKeys.List() {
+				t.Run(fmt.Sprintf("priorityfunctionkey/%s", pf), func(t *testing.T) {


I suggest removing "key" from the subtest name. Same on line 65.

misterikkit · 2018-05-11T20:18:48Z

pkg/scheduler/algorithmprovider/plugins_test.go

-			t.Errorf("Failed to find predicate: 'PodToleratesNodeTaints'")
-			break
-		}
+		t.Run(fmt.Sprintf("providername/%s", pn), func(t *testing.T) {


This test has an interesting structure. As you've currently written it, you will generate pairs of subtests with the same name. I think it would make sense to replace "providername" with a description of what the two loops are testing. Maybe call them "before_apply" and "after_apply"?

Also, I think the provider name should come first so that the sub-test names are similar to those in TestAlgorithmProviders.

misterikkit · 2018-05-11T20:19:07Z

/ok-to-test

xchapter7x · 2018-06-01T18:11:18Z

/assign @misterikkit
this PR should be in a good spot if you have a moment to take a look. thanks.

misterikkit

Looking good! just a couple comments.

misterikkit · 2018-06-02T00:13:38Z

pkg/scheduler/algorithmprovider/plugins_test.go

+			p, err := factory.GetAlgorithmProvider(pn)
+			if err != nil {
+				t.Fatalf("error retrieving provider: %v", err)
+				return


t.Fatalf causes the func to return, so return is not needed.

See https://godoc.org/testing#T.FailNow

misterikkit · 2018-06-02T00:14:51Z

pkg/scheduler/algorithmprovider/plugins_test.go

+		t.Run(pn, func(t *testing.T) {
+			p, err := factory.GetAlgorithmProvider(pn)
+			if err != nil {
+				t.Errorf("Error retrieving provider: %v", err)


replace Errorf ... return with Fatalf

implement PR feedback replace errorf + return with fatalf kubernetes#63662 (comment) kubernetes#63662 (comment)

misterikkit · 2018-06-11T23:45:10Z

/lgtm

k82cn · 2018-06-12T00:16:11Z

/approve

k8s-ci-robot · 2018-06-12T00:16:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: k82cn, misterikkit, xchapter7x

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:

~~pkg/scheduler/OWNERS~~ [k82cn]

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

fejta-bot · 2018-06-13T12:31:25Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

fejta-bot · 2018-06-18T11:51:55Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

k8s-github-robot · 2018-06-21T08:19:24Z

Automatic merge from submit-queue (batch tested with PRs 64285, 63660, 63661, 63662, 64883). If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot requested review from k82cn and wojtek-t May 10, 2018 11:27

xchapter7x mentioned this pull request May 10, 2018

Update scheduler table tests to use subtest #63281

Closed

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed 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 May 10, 2018

xchapter7x changed the title ~~[WIP] use subtest for table units (pkg/scheduler/algorithmprovider)~~ use subtest for table units (pkg/scheduler/algorithmprovider) May 10, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 10, 2018

xchapter7x mentioned this pull request May 11, 2018

scheduler: Update tests to use subtest #63267

Closed

misterikkit reviewed May 11, 2018

View reviewed changes

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 11, 2018

xchapter7x force-pushed the pkg-scheduler-algorithmprovider branch 2 times, most recently from ed647a8 to f02ab73 Compare June 1, 2018 14:47

k8s-ci-robot assigned misterikkit Jun 1, 2018

misterikkit reviewed Jun 2, 2018

View reviewed changes

use subtest for table units

f415558

implement PR feedback replace errorf + return with fatalf kubernetes#63662 (comment) kubernetes#63662 (comment)

xchapter7x force-pushed the pkg-scheduler-algorithmprovider branch from f02ab73 to f415558 Compare June 8, 2018 15:44

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 8, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 11, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 12, 2018

k8s-github-robot merged commit 15902d9 into kubernetes:master Jun 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use subtest for table units (pkg/scheduler/algorithmprovider) #63662

use subtest for table units (pkg/scheduler/algorithmprovider) #63662

xchapter7x commented May 10, 2018 •

edited

Loading

misterikkit May 11, 2018

misterikkit May 11, 2018

misterikkit May 11, 2018

misterikkit May 11, 2018

misterikkit commented May 11, 2018

xchapter7x commented Jun 1, 2018

misterikkit left a comment

misterikkit Jun 2, 2018

misterikkit Jun 2, 2018

misterikkit commented Jun 11, 2018

k82cn commented Jun 12, 2018

k8s-ci-robot commented Jun 12, 2018

fejta-bot commented Jun 13, 2018

fejta-bot commented Jun 18, 2018

k8s-github-robot commented Jun 21, 2018

use subtest for table units (pkg/scheduler/algorithmprovider) #63662

use subtest for table units (pkg/scheduler/algorithmprovider) #63662

Conversation

xchapter7x commented May 10, 2018 • edited Loading

misterikkit May 11, 2018

Choose a reason for hiding this comment

misterikkit May 11, 2018

Choose a reason for hiding this comment

misterikkit May 11, 2018

Choose a reason for hiding this comment

misterikkit May 11, 2018

Choose a reason for hiding this comment

misterikkit commented May 11, 2018

xchapter7x commented Jun 1, 2018

misterikkit left a comment

Choose a reason for hiding this comment

misterikkit Jun 2, 2018

Choose a reason for hiding this comment

misterikkit Jun 2, 2018

Choose a reason for hiding this comment

misterikkit commented Jun 11, 2018

k82cn commented Jun 12, 2018

k8s-ci-robot commented Jun 12, 2018

fejta-bot commented Jun 13, 2018

fejta-bot commented Jun 18, 2018

k8s-github-robot commented Jun 21, 2018

xchapter7x commented May 10, 2018 •

edited

Loading