E2E test for Ready->Allocated->Ready #834
Conversation
markmandel
added
the
area/tests
|
Build Succeeded 👏 Build Id: cac451a8-ae92-4c98-a2e3-1397c41cea4a 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:
|
test/e2e/framework/framework.go
Outdated
| @@ -120,7 +120,10 @@ func (f *Framework) WaitForGameServerState(gs *v1alpha1.GameServer, state v1alph | |||
| return false, nil | |||
| }) | |||
| if err != nil { | |||
| return nil, errors.Wrapf(pollErr, "waiting for GameServer to be %v %v/%v: %v", | |||
| if pollErr != nil { | |||
There was a problem hiding this comment.
Should the line 113 be updated to return the poll error instead? There is no where that PollImmediate is returning err.
There was a problem hiding this comment.
If we do that, then the retry will stop, and we could get race conditions. We may get an error because the GS isn't created yet (most likely option), but we want to wait for it to show up, and then wait for it to be Ready.
My change here allows for if the err != nil (which will happen on timeout), but the pollErr could also be nil, which means you end up with both a nil gameserver and a nil error, and very little idea what happened 😄
(This should probably have been it's own PR admittedly. I got a little lazy on this one)
There was a problem hiding this comment.
I see. Can you please add documentation on why err is replaced with pollErr?
There was a problem hiding this comment.
So I looked at this code again, and made it much simpler. I don't think we don't need the pollErr juggling - we have a warn log at that point, so we have visibility, so I got rid of it.
I think this makes the code far cleaner, and easier to read. PTAL 👍
test/e2e/framework/framework.go
Outdated
| @@ -120,7 +120,10 @@ func (f *Framework) WaitForGameServerState(gs *v1alpha1.GameServer, state v1alph | |||
| return false, nil | |||
| }) | |||
| if err != nil { | |||
| return nil, errors.Wrapf(pollErr, "waiting for GameServer to be %v %v/%v: %v", | |||
| if pollErr != nil { | |||
There was a problem hiding this comment.
I see. Can you please add documentation on why err is replaced with pollErr?
We said that using SDK.Ready() to go back to being ready once you were Allocated would work. Wrote a test to prove it actually works! Also relevant for googleforgames#660 so people can exit out of Reserved state if they need to manually do so.
markmandel
force-pushed
the
e2e/test-alloc-ready
branch
from
490ee93 to
8782a0a
Compare
| var readyGs *stable.GameServer | ||
|
|
||
| err := wait.PollImmediate(2*time.Second, timeout, func() (bool, error) { | ||
| readyGs, pollErr = f.AgonesClient.StableV1alpha1().GameServers(gs.Namespace).Get(gs.Name, metav1.GetOptions{}) | ||
| var err error | ||
| readyGs, err = f.AgonesClient.StableV1alpha1().GameServers(gs.Namespace).Get(gs.Name, metav1.GetOptions{}) |
There was a problem hiding this comment.
readyGs, err := ...
and remove var err error?
| return readyGs, nil | ||
|
|
||
| return readyGs, errors.Wrapf(err, "waiting for GameServer to be %v %v/%v", | ||
| state, gs.Namespace, gs.Name) |
There was a problem hiding this comment.
I don't think this would replace your previous change. pollErr is replaced with err which is local to the PollImmediate method.
err := wait.PollImmediate --> this error will still be timeout as it is different that the err inside PollImmediate.
There was a problem hiding this comment.
Yeah, that was my intent with this change. I'm saying I don't think we need to do replace err with pollErr at all.
The original code was in place to make the pollErr error visible, in case it is the cause of the test failing -- but we already have logging in place to show that error, so it seems redundant to do do both logging and bubble the error up.
If we remove the bubble up, the code is cleaner, and we don't sacrifice the ability to discover the reasons for issues.
At least - that is my intent 😄
|
Build Succeeded 👏 Build Id: 31051768-fcee-425d-ada5-cd01a00a2ec5 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:
|
|
Build Succeeded 👏 Build Id: 3030f74e-6123-430b-8390-75ab7eac8e41 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:
|
We said that using SDK.Ready() to go back to being ready once you were Allocated would work.
Wrote a test to prove it actually works!
Also relevant for #660 so people can exit out of Reserved state if they need to manually do so.