UPSTREAM: 63875: make TestGetServerGroupsWithTimeout more reliable #19723
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k 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 |
openshift-ci-robot
added
approved
|
/retest |
|
wait for tests to be green, LGTM otherwise |
|
@deads2k: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
|
test unit is green, this only touches unit tests. manually merging because this flake is crazy common. |
| @@ -143,18 +143,18 @@ func TestGetServerGroupsWithTimeout(t *testing.T) { | |||
| })) | |||
| defer server.Close() | |||
| defer close(done) | |||
| tmp := defaultTimeout | |||
| defaultTimeout = 2 * time.Second | |||
There was a problem hiding this comment.
That here was intentionally to ensure the default is applied out of the box. Without it, there's nothing checking that it is set. I'm ok with extending the error part, but this bit should be brought back.
There was a problem hiding this comment.
Let's sync tomorrow, I'd like to re-shape this PR before merging upstream.
There was a problem hiding this comment.
That here was intentionally to ensure the default is applied out of the box. Without it, there's nothing checking that it is set. I'm ok with extending the error part, but this bit should be brought back.
We need to check that the feature works, not that a bit of wiring does. Checking that wiring works is simply duplicating code from the original. And global state mutation in a test is a bad pattern.
There was a problem hiding this comment.
How else you want to ensure the wiring is happening, esp. if it applies only for the dynamic client, but not for regular one? I do agree that global state mutation in tests is a bad pattern, but it's quite frequent all over our code base.
There was a problem hiding this comment.
How else you want to ensure the wiring is happening, esp. if it applies only for the dynamic client, but not for regular one?
To be honest, I don't much care. If we have a problem we'll chase it and have an easy spot to wire up the fix. If the functionality works and we're short wiring a default, it's fairly easy to figure out how to connect the bits. Testing wiring is often a duplication of the code its testing.
There was a problem hiding this comment.
Discussed this in depth with David on IRC, I'm ok with this approach.
Hopefully fixes the unit test flake we're seeing.
/assign @soltysh