Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use subtest for table units (pkg/scheduler) #63661

Merged
merged 1 commit into from Jun 21, 2018

Conversation

@xchapter7x
Copy link
Contributor

xchapter7x commented May 10, 2018

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.

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

@k8s-ci-robot k8s-ci-robot requested review from davidopp and timothysc May 10, 2018

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

}
if e, a := item.expectError, gotError; !reflect.DeepEqual(e, a) {
t.Errorf("%v: error: wanted %v, got %v", i, e, a)
s, _ := NewFromConfigurator(configurator, nil...)

This comment has been minimized.

@misterikkit

misterikkit May 11, 2018

Contributor

Another weird one. nil... is the same as not passing any args after configurator. (don't fix in this PR. I'm taking notes though.)

expectError error
expectPodBind *v1.Binding
expectAssumeCalled bool
expectBindCalled bool
eventReason string
volumeBinderConfig *persistentvolume.FakeVolumeBinderConfig
}{
"all-bound": {
{
name: "all-bound",

This comment has been minimized.

@misterikkit

misterikkit May 11, 2018

Contributor

It's okay to use spaces in test names. t.Run will replace them with underscore.

@@ -694,7 +763,8 @@ func TestSchedulerWithVolumeBinding(t *testing.T) {

eventReason: "Scheduled",
},
"bound,invalid-pv-affinity": {
{
name: "bound,invalid-pv-affinity",

This comment has been minimized.

@misterikkit

misterikkit May 11, 2018

Contributor

The commas could probably be replaced with slashes in these test names. (I'm assuming you already know how -run interacts with subtests and subsubtests.)

@misterikkit

This comment has been minimized.

Copy link
Contributor

misterikkit commented May 11, 2018

/ok-to-test

@xchapter7x xchapter7x force-pushed the xchapter7x:pkg-scheduler branch from 8f3661f to 90aa2fd May 12, 2018

xchapter7x added a commit to xchapter7x/kubernetes that referenced this pull request May 12, 2018

@xchapter7x

This comment has been minimized.

Copy link
Contributor Author

xchapter7x commented May 12, 2018

/test pull-kubernetes-integration

@xchapter7x

This comment has been minimized.

Copy link
Contributor Author

xchapter7x commented May 12, 2018

/test pull-kubernetes-integration
/test pull-kubernetes-e2e-gce

@timothysc timothysc removed their request for review May 14, 2018

@xchapter7x

This comment has been minimized.

Copy link
Contributor Author

xchapter7x commented May 25, 2018

/assign @davidopp

@xchapter7x xchapter7x force-pushed the xchapter7x:pkg-scheduler branch from 90aa2fd to 27c080d May 28, 2018

xchapter7x added a commit to xchapter7x/kubernetes that referenced this pull request May 28, 2018

@xchapter7x

This comment has been minimized.

Copy link
Contributor Author

xchapter7x commented Jun 1, 2018

/test pull-kubernetes-e2e-kops-aws
/test pull-kubernetes-bazel-build
/test pull-kubernetes-e2e-gce-device-plugin-gpu

@xchapter7x

This comment has been minimized.

Copy link
Contributor Author

xchapter7x commented Jun 1, 2018

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

}

func testScheduler(
injectBindError error,

This comment has been minimized.

@misterikkit

misterikkit Jun 2, 2018

Contributor

As we discussed in #63665 (comment), let's keep this inline for now.

@@ -381,53 +421,80 @@ func TestSchedulerErrorWithLongBinding(t *testing.T) {
conflictPod := podWithPort("bar", "", 8080)
pods := map[string]*v1.Pod{firstPod.Name: firstPod, conflictPod.Name: conflictPod}
for _, test := range []struct {
Name string

This comment has been minimized.

@misterikkit

misterikkit Jun 2, 2018

Contributor

lowercase name

@xchapter7x xchapter7x force-pushed the xchapter7x:pkg-scheduler branch from 27c080d to d29444e Jun 8, 2018

xchapter7x added a commit to xchapter7x/kubernetes that referenced this pull request Jun 8, 2018

use subtest for table units
  apply consistent format to name strings

  - kubernetes#63661 (comment)
  - kubernetes#63661 (comment)

inline subtest logic

  https://github.com/kubernetes/kubernetes/pull/63661/files#r192540031
use subtest for table units
  apply consistent format to name strings

  - #63661 (comment)
  - #63661 (comment)

inline subtest logic

  https://github.com/kubernetes/kubernetes/pull/63661/files#r192540031

remove duplicate messaging in subtest errors

@xchapter7x xchapter7x force-pushed the xchapter7x:pkg-scheduler branch from d29444e to 7735dd6 Jun 8, 2018

@xchapter7x

This comment has been minimized.

Copy link
Contributor Author

xchapter7x commented Jun 8, 2018

/test pull-kubernetes-e2e-gce-100-performance

@misterikkit

This comment has been minimized.

Copy link
Contributor

misterikkit commented Jun 11, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Jun 11, 2018

@bsalamat
Copy link
Contributor

bsalamat left a comment

/approve

Thanks, @xchapter7x!

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jun 12, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, 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:

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

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Jun 13, 2018

/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.

1 similar comment
@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Jun 18, 2018

/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

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jun 21, 2018

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-github-robot k8s-github-robot merged commit 5857402 into kubernetes:master Jun 21, 2018

18 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation xchapter7x authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.