Convert TestServerRunWithSNI to subtests to isolate flake #66008
Conversation
|
@smarterclayton: 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/test-infra repository. |
k8s-ci-robot
added
do-not-merge/release-note-label-needed
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: smarterclayton 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 |
k8s-ci-robot
added
the
approved
k8s-ci-robot
added
the
cncf-cla: yes
| certBundleFile := filepath.Join(sniDir, "cert") | ||
| keyFile := filepath.Join(sniDir, "key") | ||
| err := getOrCreateTestCertFiles(certBundleFile, keyFile, c.TestCertSpec) | ||
| for title := range tests { |
There was a problem hiding this comment.
why not for title, test := range tests {?
There was a problem hiding this comment.
oh, right. There was something (ugly) in Golang: loop vars in a closure are no copies.
| @@ -554,28 +545,25 @@ NextTest: | |||
| s.LoopbackClientConfig.Host = net.JoinHostPort(host, strconv.Itoa(secureOptions.BindPort)) | |||
| if test.ExpectLoopbackClientError { | |||
| if err == nil { | |||
| t.Errorf("%q - expected error creating loopback client config", title) | |||
| t.Fatalf("%q - expected error creating loopback client config", title) | |||
|
Some nit. lgtm when fixed. |
This test is flaking - make it easier to pin down where and why by converting to subtests and making cleanup logic easier. Also turn an ignored listen error into a "fatal". Make the test run in parallel to speed up individual runs and hopefully flush out issues.
smarterclayton
force-pushed
the
serving_test
branch
from
b4aeb5a
to
0946397
Compare
smarterclayton
added
release-note-none
|
Nit fixed, applied label |
smarterclayton
removed
the
do-not-merge/release-note-label-needed
|
Automatic merge from submit-queue (batch tested with PRs 66038, 65992, 66008). If you want to cherry-pick this change to another branch, please follow the instructions here. |
This test is flaking - make it easier to pin down where and why by
converting to subtests and making cleanup logic simpler. Also turn an
ignored listen error into a "fatal".
Make the test run in parallel to speed up individual runs and hopefully
flush out issues.
Noticed and reported in OpenShift, openshift/origin#20220
@deads2k / @sttts