Flaky: TestSDKServerWatchGameServerFeatureSDKWatchSendOnExecute #1667
Conversation
markmandel
added
kind/cleanup
pkg/sdkserver/sdkserver_test.go
Outdated
| @@ -718,7 +718,7 @@ func TestSDKServerWatchGameServerFeatureSDKWatchSendOnExecute(t *testing.T) { | |||
| } else { | |||
| assert.Fail(t, "Channel is closed!") | |||
| } | |||
| default: | |||
| case <-time.After(30 * time.Second): | |||
There was a problem hiding this comment.
Does this still need to try 2 times? If you are going to add a timeout, maybe remove the for statement?
There was a problem hiding this comment.
This test is looking for two events to be passed through - the initial one on connection, then the second one from WatchGameServer.
I could rewrite it as two select statements, but that feels a little verbose?
There was a problem hiding this comment.
I didn't catch that part. I think it might be a bit clearer and/or more idiomatic to do a for { } loop with a break (when it times out or when you've gotten the expected events) but this is ok too.
There was a problem hiding this comment.
Would there be a concern that it would never escape the loop though?
There was a problem hiding this comment.
You would escape the loop as long as you add break / return (or panic) statements to each case, e.g.
for {
select {
case _, ok := <-stream.msgs:
// do stuff - maybe assert that the right messages show up?
break
case <-time.After(timeout):
// error, since we hit a timeout when we were expecting messages
t.Fail() // should cause the test to end, so no need to break / return
}
}
|
Build Succeeded 👏 Build Id: ca5520c2-d9c1-4826-875a-d341304f9fd0 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
| @@ -700,6 +699,8 @@ func TestSDKServerWatchGameServerFeatureSDKWatchSendOnExecute(t *testing.T) { | |||
| stop := make(chan struct{}) | |||
| defer close(stop) | |||
| sc.informerFactory.Start(stop) | |||
|
|
|||
| fakeWatch.Add(fixture.DeepCopy()) | |||
There was a problem hiding this comment.
Using a fake Watch so we can control explicitly when the game server is modified.
| @@ -709,17 +710,31 @@ func TestSDKServerWatchGameServerFeatureSDKWatchSendOnExecute(t *testing.T) { | |||
| assert.Nil(t, waitConnectedStreamCount(sc, 1)) | |||
| assert.Equal(t, stream, sc.connectedStreams[0]) | |||
|
|
|||
| // modify for 2nd event in watch stream | |||
| fixture.Status.State = agonesv1.GameServerStateAllocated | |||
There was a problem hiding this comment.
@roberthbailey @akremsa - I reworked this test to check for:
- We get what we expect in events on watch and modification
- That we don't get too many events
- We can't go into an infinite loop
PTAL, let me know what you think
There was a problem hiding this comment.
Nice update! I like the way you've updated a select section.
There was a problem hiding this comment.
I also suggested to use state other than Ready.
Switching default: case to a timeout, since due to the asynchronous nature of event processing, it could take a moment or two for the extra event to be processed. This is particularly true on the CI system, where there is a lot of processing going on, and it may take more time than usual to process.
markmandel
force-pushed
the
flaky/TestSDKServerWatchGameServerFeatureSDKWatchSendOnExecute
branch
from
cf7cf12
to
1ce642d
Compare
|
Build Succeeded 👏 Build Id: 33b634a5-675b-43c3-a112-3e0d56307c93 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: markmandel, roberthbailey 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 |
markmandel
deleted the
flaky/TestSDKServerWatchGameServerFeatureSDKWatchSendOnExecute
branch
…leforgames#1667) * Flaky: TestSDKServerWatchGameServerFeatureSDKWatchSendOnExecute Switching default: case to a timeout, since due to the asynchronous nature of event processing, it could take a moment or two for the extra event to be processed. This is particularly true on the CI system, where there is a lot of processing going on, and it may take more time than usual to process.
What type of PR is this?
/kind cleanup
What this PR does / Why we need it:
Switching default: case to a timeout, since due to the asynchronous nature of event processing, it could take a moment or two for the extra event to be processed.
This is particularly true on the CI system, where there is a lot of processing going on, and it may take more time than usual to process.
Which issue(s) this PR fixes:
Closes #
Special notes for your reviewer: