test-e2e - Added several Fatal calls #1629

akremsa · 2020-06-17T13:27:56Z

What type of PR is this?

/kind bug
/kind cleanup

What this PR does / Why we need it:
Added those additional Fatal calls in order to avoid nil pointer exceptions in some places.

agones-bot · 2020-06-17T13:39:11Z

Build Failed 😱

Build Id: 3a0cdc27-aa72-4d91-8ccd-9c6e1f74dcd9

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

agones-bot · 2020-06-17T14:34:02Z

Build Succeeded 👏

Build Id: 286498f2-7a32-410a-ba75-ddd479f5b360

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.7.0-99f9ffe
image: gcr.io/agones-images/agones-sdk:1.7.0-99f9ffe
image: gcr.io/agones-images/agones-ping:1.7.0-99f9ffe
Linux C++ SDK (build): agonessdk-1.7.0-99f9ffe-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.7.0-99f9ffe.zip

A preview of the website (the last 30 builds are retained):

https://99f9ffe-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/1629/head:pr_1629 && git checkout pr_1629
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.7.0-99f9ffe

agones-bot · 2020-06-19T13:41:27Z

Build Failed 😱

Build Id: 647b924e-e574-461e-a2e0-79a4b6082ddd

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

agones-bot · 2020-06-19T14:07:22Z

Build Succeeded 👏

Build Id: 84f66a8f-816d-471c-8958-843dc5d0a698

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.7.0-a1ea40c
image: gcr.io/agones-images/agones-sdk:1.7.0-a1ea40c
image: gcr.io/agones-images/agones-ping:1.7.0-a1ea40c
Linux C++ SDK (build): agonessdk-1.7.0-a1ea40c-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.7.0-a1ea40c.zip

A preview of the website (the last 30 builds are retained):

https://a1ea40c-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/1629/head:pr_1629 && git checkout pr_1629
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.7.0-a1ea40c

test/e2e/gameserver_test.go

markmandel · 2020-06-22T23:18:32Z

test/e2e/gameserver_test.go

 	assert.Equal(t, "3\n", reply)

 	err = wait.Poll(time.Second, time.Minute, func() (bool, error) {
 		gs, err = framework.AgonesClient.AgonesV1().GameServers(framework.Namespace).Get(gs.ObjectMeta.Name, metav1.GetOptions{})
-		if err != nil {
+		if !assert.NoError(t, err) {


Same as all the above - I don't think these are providing any kind of extra value, and could be confusing.

There are places where we check errors with assert library in tests, that is why I decided to do the same:
https://github.com/googleforgames/agones/blob/master/pkg/util/apiserver/apiserver_test.go#L100
https://github.com/googleforgames/agones/blob/master/test/e2e/fleetautoscaler_test.go#L358
https://github.com/googleforgames/agones/blob/master/test/e2e/gameserverallocation_test.go#L356

As I see, currently there is no single approach and we use both options across the project. You are right, there is no big difference, but as far as this is a test file I decided to use a testing library here.
In which cases do you think we should use err != nil or assert.NoError() ?

I don't know if I have strong opinions on either one. @roberthbailey I feel like you had opinions on this at one stage.

I feel like consolidating to:

if !assert.NoError(t, err) { assert.FailNow(t, "the is my message for why this failed") }

Feels like the solution that best takes advantage of the test libraries we use, but that I think Robbie has preferred if err != nil more?

Thoughts?

It's redundant to both assert.NoError and then call Fail, since the former calls fail already: https://github.com/stretchr/testify/blob/master/assert/assertions.go#L1327

The one place where I don't recommend using the assert calls in tests is in goroutines, since calling asserts there don't necessarily bubble up nicely back to the main testing thread and give you a good idea of why a test failed. In those cases, doing "normal" error checking (if err != nil) and returning regular errors from the goroutines and then doing the asserts when they come back to the main routine is generally nicer. This is where I generally add comments about doing the normal error checking.

It's redundant to both assert.NoError and then call Fail, since the former calls fail already:

But FailNow will stop the test at that specific moment, which I think is the point. Or am I missing the point of your comment?

Yes, aborting the test in case of an error is an idea, because if we go further nil value will be used. As a result, I’ve noticed nil pointer exceptions in some tests along with assert-fail messages in case if test fails.

If we assert twice then we will duplicate error logs in the test output. So if we need to fail now I'm in favor of doing a regular if err != nil check to reduce noise in the test logs.

akremsa · 2020-06-23T12:33:22Z

@markmandel I've updated PR taking into account your comments. Please check.

agones-bot · 2020-06-23T12:53:24Z

Build Succeeded 👏

Build Id: 7f2a3504-c85b-41c2-b52f-f308c72ae174

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.7.0-a748bd0
image: gcr.io/agones-images/agones-sdk:1.7.0-a748bd0
image: gcr.io/agones-images/agones-ping:1.7.0-a748bd0
Linux C++ SDK (build): agonessdk-1.7.0-a748bd0-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.7.0-a748bd0.zip

A preview of the website (the last 30 builds are retained):

https://a748bd0-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/1629/head:pr_1629 && git checkout pr_1629
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.7.0-a748bd0

We need them in order to avoid nil pointer exceptions in some places fix

agones-bot · 2020-06-25T15:33:06Z

Build Failed 😱

Build Id: f3858f7b-07a0-4f42-9b66-5e2e38839185

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

agones-bot · 2020-06-25T15:33:34Z

Build Succeeded 👏

Build Id: ac3bdf3e-b580-4b66-8d2a-ae5a05c1e443

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.7.0-4e2ba2f
image: gcr.io/agones-images/agones-sdk:1.7.0-4e2ba2f
image: gcr.io/agones-images/agones-ping:1.7.0-4e2ba2f
Linux C++ SDK (build): agonessdk-1.7.0-4e2ba2f-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.7.0-4e2ba2f.zip

A preview of the website (the last 30 builds are retained):

https://4e2ba2f-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/1629/head:pr_1629 && git checkout pr_1629
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.7.0-4e2ba2f

agones-bot · 2020-06-25T16:10:09Z

Build Succeeded 👏

Build Id: a190d042-4f3e-4782-b178-b9210305dd31

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.7.0-a2e2b68
image: gcr.io/agones-images/agones-sdk:1.7.0-a2e2b68
image: gcr.io/agones-images/agones-ping:1.7.0-a2e2b68
Linux C++ SDK (build): agonessdk-1.7.0-a2e2b68-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.7.0-a2e2b68.zip

A preview of the website (the last 30 builds are retained):

https://a2e2b68-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/1629/head:pr_1629 && git checkout pr_1629
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.7.0-a2e2b68

agones-bot · 2020-06-25T16:14:47Z

Build Failed 😱

Build Id: c5f3c455-c01e-437e-bdc0-775ace4557c3

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

agones-bot · 2020-06-25T16:15:37Z

Build Succeeded 👏

Build Id: 123eb4f7-fdf5-4c83-9f71-c03d63d75321

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:1.7.0-e461950
image: gcr.io/agones-images/agones-sdk:1.7.0-e461950
image: gcr.io/agones-images/agones-ping:1.7.0-e461950
Linux C++ SDK (build): agonessdk-1.7.0-e461950-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.7.0-e461950.zip

A preview of the website (the last 30 builds are retained):

https://e461950-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/1629/head:pr_1629 && git checkout pr_1629
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.7.0-e461950

akremsa · 2020-06-25T16:20:50Z

Updated again. I've replaced !assert.NoError with err != nil and put error message to FailNow call.

markmandel

I like it! LGTM!

google-oss-robot · 2020-06-25T20:15:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akremsa, markmandel

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:

~~OWNERS~~ [markmandel]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* test-e2e - Added several Fatal calls We need them in order to avoid nil pointer exceptions in some places

google-oss-robot added the do-not-merge/work-in-progress label Jun 17, 2020

google-oss-robot requested review from cyriltovena and markmandel June 17, 2020 13:27

googlebot added the cla: yes label Jun 17, 2020

google-oss-robot added the size/L label Jun 17, 2020

akremsa force-pushed the gameserver_test_refactoring branch from 871960d to 99f9ffe Compare June 17, 2020 14:11

akremsa marked this pull request as ready for review June 17, 2020 15:01

google-oss-robot removed the do-not-merge/work-in-progress label Jun 17, 2020

akremsa force-pushed the gameserver_test_refactoring branch from 99f9ffe to d04393f Compare June 19, 2020 13:34

akremsa force-pushed the gameserver_test_refactoring branch from d04393f to a1ea40c Compare June 19, 2020 13:45

markmandel reviewed Jun 22, 2020

View reviewed changes

akremsa force-pushed the gameserver_test_refactoring branch from a1ea40c to a748bd0 Compare June 23, 2020 12:32

test-e2e - Added several Fatal calls

7e41cdc

We need them in order to avoid nil pointer exceptions in some places fix

akremsa force-pushed the gameserver_test_refactoring branch 3 times, most recently from 5b25a6f to e7f1605 Compare June 25, 2020 15:32

akremsa force-pushed the gameserver_test_refactoring branch from e7f1605 to a2e2b68 Compare June 25, 2020 15:36

applied review notes

e461950

akremsa force-pushed the gameserver_test_refactoring branch from a2e2b68 to e461950 Compare June 25, 2020 15:39

markmandel approved these changes Jun 25, 2020

View reviewed changes

google-oss-robot assigned markmandel Jun 25, 2020

google-oss-robot added the lgtm label Jun 25, 2020

google-oss-robot added the approved label Jun 25, 2020

markmandel merged commit 4c857b1 into googleforgames:master Jun 25, 2020

markmandel added this to the 1.7.0 milestone Jun 25, 2020

markmandel added area/tests Unit tests, e2e tests, anything to make sure things don't break kind/cleanup Refactoring code, fixing up documentation, etc labels Jun 25, 2020

akremsa mentioned this pull request Jun 30, 2020

Added missing FailNow calls to sdkserver unit tests #1659

Merged

ilkercelikyilmaz pushed a commit to ilkercelikyilmaz/agones that referenced this pull request Oct 23, 2020

test-e2e - Added several Fatal calls (googleforgames#1629)

acf3f77

* test-e2e - Added several Fatal calls We need them in order to avoid nil pointer exceptions in some places

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test-e2e - Added several Fatal calls #1629

test-e2e - Added several Fatal calls #1629

akremsa commented Jun 17, 2020 •

edited

Loading

agones-bot commented Jun 17, 2020

agones-bot commented Jun 17, 2020

agones-bot commented Jun 19, 2020

agones-bot commented Jun 19, 2020

markmandel Jun 22, 2020

akremsa Jun 23, 2020

markmandel Jun 24, 2020

roberthbailey Jun 24, 2020

markmandel Jun 24, 2020

akremsa Jun 24, 2020

roberthbailey Jun 24, 2020

akremsa commented Jun 23, 2020

agones-bot commented Jun 23, 2020

agones-bot commented Jun 25, 2020

agones-bot commented Jun 25, 2020

agones-bot commented Jun 25, 2020

agones-bot commented Jun 25, 2020

agones-bot commented Jun 25, 2020

akremsa commented Jun 25, 2020

markmandel left a comment

google-oss-robot commented Jun 25, 2020

test-e2e - Added several Fatal calls #1629

test-e2e - Added several Fatal calls #1629

Conversation

akremsa commented Jun 17, 2020 • edited Loading

agones-bot commented Jun 17, 2020

agones-bot commented Jun 17, 2020

agones-bot commented Jun 19, 2020

agones-bot commented Jun 19, 2020

markmandel Jun 22, 2020

Choose a reason for hiding this comment

akremsa Jun 23, 2020

Choose a reason for hiding this comment

markmandel Jun 24, 2020

Choose a reason for hiding this comment

roberthbailey Jun 24, 2020

Choose a reason for hiding this comment

markmandel Jun 24, 2020

Choose a reason for hiding this comment

akremsa Jun 24, 2020

Choose a reason for hiding this comment

roberthbailey Jun 24, 2020

Choose a reason for hiding this comment

akremsa commented Jun 23, 2020

agones-bot commented Jun 23, 2020

agones-bot commented Jun 25, 2020

agones-bot commented Jun 25, 2020

agones-bot commented Jun 25, 2020

agones-bot commented Jun 25, 2020

agones-bot commented Jun 25, 2020

akremsa commented Jun 25, 2020

markmandel left a comment

Choose a reason for hiding this comment

google-oss-robot commented Jun 25, 2020

akremsa commented Jun 17, 2020 •

edited

Loading