Improve the failure mode of timeout_test. #3396
Conversation
tekton-robot
added
the
release-note-none
tekton-robot
added
the
size/M
|
/kind cleanup |
tekton-robot
added
the
kind/cleanup
|
/test check-pr-has-kind-label |
|
Looks like there may be some bad assumption here that I need to dig into 😞 I'm out of time though before vacay, so if anyone want to pick this up, feel free, otherwise I'll come back to it when I'm back. |
I notice that the failure mode for `timeout_test.go` tests on flakes seems to often be hitting the Go `-timeout=X` limit, which means that without `-v` you get no logs for the test. This change makes the tests use a context with a Timeout, and makes the various `Wait` functions check the `context.Done()` and return `context.Err()` to support the timeout terminating the test earlier than the above and producing logs (other than an ugly panic!).
mattmoor
force-pushed
the
better-timeouts
branch
from
11c9603
to
9f964d6
Compare
tekton-robot
added
size/L
|
Alright, so I also changed the |
|
Alright, this should be RFAL... /assign @afrittoli @vdemeester @imjasonh |
test/wait.go
Outdated
| @@ -147,6 +167,11 @@ func WaitForServiceExternalIPState(ctx context.Context, c *clients, namespace, n | |||
| defer span.End() | |||
|
|
|||
| return wait.PollImmediate(interval, timeout, func() (bool, error) { | |||
| select { | |||
There was a problem hiding this comment.
I'm worried future callers will forget this stanza at the top of their funcs. Can we somehow make this the responsibility of PollImmediate so it's not the caller's responsibility?
There was a problem hiding this comment.
I think I have another shape in mind that you might be happier with, gimme a few minutes 😉
There was a problem hiding this comment.
It ends up being gross too, lemme write a shared helper...
There was a problem hiding this comment.
alright, I think I pushed a change that better centralizes the context cancellation handling logic
mattmoor
force-pushed
the
better-timeouts
branch
from
27925a1
to
4a38a12
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vdemeester 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 |
tekton-robot
added
the
approved
|
/lgtm |
|
couldn't resolve host github.com... /retest |
|
Weird, when I click the details links, all the builds say still running. Let's try this again... /retest |
Changes
I notice that the failure mode for
timeout_test.gotests on flakes seems to often be hitting the Go-timeout=Xlimit, which means that without-vyou get no logs for the test.This change makes the tests use a context with a Timeout, and makes the various
Waitfunctions check thecontext.Done()and returncontext.Err()to support the timeout terminating the test earlier than the above and producing logs (other than an ugly panic!).Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Double check this list of stuff that's easy to miss:
cmddir, please updatethe release Task to build and release this image.
Reviewer Notes
If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.
Release Notes