Have TestPlayerConnectWithCapacityZero use framework to wait #3062
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: gongmax, zmerlynn The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
FYI: |
|
|
||
| return assert.Equal(t, gs.Status.State, agonesv1.GameServerStateUnhealthy) | ||
| }, time.Minute, time.Second) | ||
| if assert.Error(t, err) { |
There was a problem hiding this comment.
This if statement is doing exactly the same thing require.NoError does 😄
There was a problem hiding this comment.
And then panicking via FailNow: https://github.com/stretchr/testify/blob/v1.8.2/require/require.go#L1257-L1265
There was a problem hiding this comment.
Actually - digging into FailNow, actually explains things!!!
https://pkg.go.dev/testing#T.FailNow
(emphasis mine)
FailNow marks the function as having failed and stops its execution by calling runtime.Goexit (which then runs all deferred calls in the current goroutine). Execution will continue at the next test or benchmark. FailNow must be called from the goroutine running the test or benchmark function, not from other goroutines created during the test. Calling FailNow does not stop those other goroutines.
There was a problem hiding this comment.
Right.. and testify does doc that: https://github.com/stretchr/testify/tree/v1.8.2#require-package
Basically require is to assert as t.Fatal is to t.Error, AFAICT.
There was a problem hiding this comment.
The thing that doesn't make sense... is Eventually shouldn't be starting a go routine? Or does it?
There was a problem hiding this comment.
Yeah that one I didn't look up yet, but it sure does: https://github.com/stretchr/testify/blob/v1.8.2/assert/assertions.go#L1737
How weird!
There was a problem hiding this comment.
I think it's so the timeout has precedence over the condition - i.e. if you set a 1m timeout, it always fires if the condition takes too long to evaluate.
There was a problem hiding this comment.
...actually, it 100% does.
This seems like a bug in testify to me tbh.
There was a problem hiding this comment.
Yeah.. the framework code just leans on https://pkg.go.dev/k8s.io/apimachinery/pkg/util/wait
Given that stack traces, it seems to be causing some nasty interaction, especially in the |
|
Might need to file a bug on testify! |
|
Build Failed 😱 Build Id: 4e5d44e1-8d24-433b-9d33-9af929dc9291 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
|
New changes are detected. LGTM label has been removed. |
I'm seeing e2e failures in this test, which I'm trying to track
down, but it seems like when this test fails, it explodes, e.g.:
panic: Fail in goroutine after TestPlayerConnectWithCapacityZero has completed
[...]
agones.dev/agones/test/e2e.TestPlayerConnectWithCapacityZero.func1()
/go/src/agones.dev/agones/test/e2e/gameserver_test.go:1111 +0x2d3
I think this is largely due to using `require`, which panics. Instead,
we have a framework function that does exactly what this is looping
on, just use that.
zmerlynn
force-pushed
the
use-framework-instead-of-eventually
branch
from
2e558a7 to
1d5d431
Compare
|
Build Succeeded 👏 Build Id: 60534a1d-4304-4893-82b4-2535817b0ce3 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: 1f52f19f-1b6d-450d-9cdb-df5cfec19bdc 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:
|
…orgames#3062) I'm seeing e2e failures in this test, which I'm trying to track down, but it seems like when this test fails, it explodes, e.g.: panic: Fail in goroutine after TestPlayerConnectWithCapacityZero has completed [...] agones.dev/agones/test/e2e.TestPlayerConnectWithCapacityZero.func1() /go/src/agones.dev/agones/test/e2e/gameserver_test.go:1111 +0x2d3 I think this is largely due to using `require`, which panics. Instead, we have a framework function that does exactly what this is looping on, just use that.
…orgames#3062) I'm seeing e2e failures in this test, which I'm trying to track down, but it seems like when this test fails, it explodes, e.g.: panic: Fail in goroutine after TestPlayerConnectWithCapacityZero has completed [...] agones.dev/agones/test/e2e.TestPlayerConnectWithCapacityZero.func1() /go/src/agones.dev/agones/test/e2e/gameserver_test.go:1111 +0x2d3 I think this is largely due to using `require`, which panics. Instead, we have a framework function that does exactly what this is looping on, just use that.
I'm seeing e2e failures in this test, which I'm trying to track down, but it seems like when this test fails, it explodes, e.g.:
It then ends up wrecking status of several other tests and makes the output noisier than it actually is. I think this is largely due to using
require, which panics. Instead, we have a framework function that does exactly what this is looping on, just use that.