Improved gameserver unit tests #1485

akremsa · 2020-04-22T13:06:52Z

What type of PR is this?

/kind cleanup

What this PR does / Why we need it:
Added missing unit tests, refactored some old ones. During working on tests I've found a missing check in Validate method, which is related to #1396

Before:

After:

akremsa · 2020-04-22T13:13:23Z

pkg/apis/agones/v1/gameserver.go

@@ -572,22 +584,13 @@ func (gs *GameServer) FindGameServerContainer() (int, corev1.Container, error) {
 // ApplyToPodContainer applies func(v1.Container) to the specified container in the pod.
 // Returns an error if the container is not found.
 func (gs *GameServer) ApplyToPodContainer(pod *corev1.Pod, containerName string, f func(corev1.Container) corev1.Container) error {
-	var container corev1.Container


Just decided to make this code a bit easier. In the previous implementation, containerIndex was the last match of container names. But in general, having 2 containers with the same name is not valid, so it's ok to return the first occurrence.

There is a Pod validation which prohibit having two containers in one Pod with the same name.

agones-bot · 2020-04-22T13:23:52Z

Build Succeeded 👏

Build Id: a1e6b383-b2e3-4054-b670-c759b69fcb9a

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

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

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

https://250ec0c-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1485/head:pr_1485 && git checkout pr_1485
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.6.0-250ec0c

aLekSer · 2020-04-22T15:58:38Z

pkg/apis/agones/v1/gameserver.go

 			})
 		}
 	}

+	if !runtime.FeatureEnabled(runtime.FeatureContainerPortAllocation) {


That's a great way to separate this Feature logic.

aLekSer · 2020-04-22T15:59:03Z

pkg/apis/agones/v1/gameserver.go

@@ -439,7 +451,7 @@ func (gss *GameServerSpec) Validate(devAddress string) ([]metav1.StatusCause, bo
 				})
 			}

-			if p.Container != nil && gss.Container != "" {
+			if p.Container != nil && gss.Container != "" && runtime.FeatureEnabled(runtime.FeatureContainerPortAllocation) {


pkg/apis/agones/v1/gameserver_test.go

markmandel

One question on one test, but otherwise looks good.

markmandel · 2020-04-23T23:58:43Z

pkg/apis/agones/v1/gameserver_test.go

@@ -358,17 +427,22 @@ func TestGameServerApplyDefaults(t *testing.T) {
 }

 func TestGameServerValidate(t *testing.T) {
-	gs := GameServer{
+	t.Parallel()
+	var fields []string


Should this be turned into a table based test, since it's grown so large?

I thought about it... but wasn't sure if it is worth spending extra time on that. But once you mentioned it - let's do that.

added more gameserver unit tests Added new tests

fix

pkg/apis/agones/v1/common.go

agones-bot · 2020-04-25T11:15:54Z

Build Failed 😱

Build Id: 8e9091d4-442d-4f6c-becd-a3599093cdb2

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

akremsa · 2020-04-25T11:19:14Z

@markmandel I've refactored Validate method and extracted tests which test features functionality to a separate function. Please check.

agones-bot · 2020-04-25T11:25:47Z

Build Failed 😱

Build Id: af34469b-a2de-4e47-89f3-ec7d1fbb7429

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

agones-bot · 2020-04-25T11:52:40Z

Build Failed 😱

Build Id: 1a9c330e-57c0-4f99-bb27-79d56c2cb823

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

valdiate refactoring valdiate method refactoring removed redundant file linter fix

agones-bot · 2020-04-25T12:11:54Z

Build Succeeded 👏

Build Id: 9feaa4cc-b710-4c11-b5ac-af5eaf226be5

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

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

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

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

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1485/head:pr_1485 && git checkout pr_1485
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.6.0-e9acbf5

pkg/apis/agones/v1/common.go

pkg/apis/agones/v1/gameserver_test.go

markmandel · 2020-04-27T17:53:51Z

pkg/apis/agones/v1/gameserver_test.go

+	var testCases = []struct {
+		description string
+		gs          *GameServer
+		errExpected string


Something to consider: Could pull the expected items into their own struct definition. I find it's a bit easier to read when there are > 1 expected items.

Don't have to do this, but thought I'd suggest it and see if it resonated for you.

I like this idea - nice way to separate expected values. Applied all your suggestions, thanks for the review.

agones-bot · 2020-04-27T21:34:23Z

Build Failed 😱

Build Id: 9a7861ba-a754-4816-8d6c-f71a852d04f2

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

applied review notes

agones-bot · 2020-04-27T21:56:54Z

Build Succeeded 👏

Build Id: a3751db7-b5d5-4e48-aa9b-f20fbb5a9cef

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

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

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

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

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1485/head:pr_1485 && git checkout pr_1485
helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.6.0-ddf8d83

google-oss-robot · 2020-04-28T15:43:53Z

[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

* added some gameserver unit tests

google-oss-robot added the size/XL label Apr 22, 2020

google-oss-robot requested review from dzlier-gcp and markmandel April 22, 2020 13:07

akremsa commented Apr 22, 2020

View reviewed changes

aLekSer reviewed Apr 22, 2020

View reviewed changes

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

aLekSer reviewed Apr 22, 2020

View reviewed changes

pkg/apis/agones/v1/gameserver_test.go Outdated Show resolved Hide resolved

akremsa force-pushed the pkg-apis-unittests branch from 250ec0c to 2847dd4 Compare April 23, 2020 12:58

markmandel reviewed Apr 23, 2020

View reviewed changes

alexey-kremsa-globant added 2 commits April 25, 2020 14:11

added some gameserver unit tests

9b77254

added more gameserver unit tests Added new tests

Added missing FeatureEnabled check to Validate method

f57dbe2

fix

akremsa force-pushed the pkg-apis-unittests branch from 2847dd4 to a3ebea4 Compare April 25, 2020 11:11

google-oss-robot added size/XXL and removed size/XL labels Apr 25, 2020

akremsa commented Apr 25, 2020

View reviewed changes

pkg/apis/agones/v1/common.go Outdated Show resolved Hide resolved

akremsa force-pushed the pkg-apis-unittests branch from a3ebea4 to 9d41a42 Compare April 25, 2020 11:18

akremsa force-pushed the pkg-apis-unittests branch from 9d41a42 to 674d80a Compare April 25, 2020 11:49

gameserver Validate test refactoring

e9acbf5

valdiate refactoring valdiate method refactoring removed redundant file linter fix

akremsa force-pushed the pkg-apis-unittests branch from 674d80a to e9acbf5 Compare April 25, 2020 11:54

markmandel reviewed Apr 27, 2020

View reviewed changes

applied review notes

ddf8d83

applied review notes

akremsa force-pushed the pkg-apis-unittests branch from c308919 to ddf8d83 Compare April 27, 2020 21:39

markmandel approved these changes Apr 28, 2020

View reviewed changes

google-oss-robot assigned markmandel Apr 28, 2020

google-oss-robot added the lgtm label Apr 28, 2020

google-oss-robot added the approved label Apr 28, 2020

markmandel merged commit 233d0ff into googleforgames:master Apr 28, 2020

markmandel added this to the 1.6.0 milestone Apr 28, 2020

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

Improved gameserver unit tests (googleforgames#1485)

ca5c349

* added some gameserver unit tests

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved gameserver unit tests #1485

Improved gameserver unit tests #1485

akremsa commented Apr 22, 2020 •

edited

Loading

akremsa Apr 22, 2020 •

edited

Loading

aLekSer Apr 22, 2020

agones-bot commented Apr 22, 2020

aLekSer Apr 22, 2020

aLekSer Apr 22, 2020

markmandel left a comment

markmandel Apr 23, 2020

akremsa Apr 24, 2020

agones-bot commented Apr 25, 2020

akremsa commented Apr 25, 2020 •

edited

Loading

agones-bot commented Apr 25, 2020

agones-bot commented Apr 25, 2020

agones-bot commented Apr 25, 2020

markmandel Apr 27, 2020

akremsa Apr 27, 2020

agones-bot commented Apr 27, 2020

agones-bot commented Apr 27, 2020

google-oss-robot commented Apr 28, 2020

Improved gameserver unit tests #1485

Improved gameserver unit tests #1485

Conversation

akremsa commented Apr 22, 2020 • edited Loading

akremsa Apr 22, 2020 • edited Loading

Choose a reason for hiding this comment

aLekSer Apr 22, 2020

Choose a reason for hiding this comment

agones-bot commented Apr 22, 2020

aLekSer Apr 22, 2020

Choose a reason for hiding this comment

aLekSer Apr 22, 2020

Choose a reason for hiding this comment

markmandel left a comment

Choose a reason for hiding this comment

markmandel Apr 23, 2020

Choose a reason for hiding this comment

akremsa Apr 24, 2020

Choose a reason for hiding this comment

agones-bot commented Apr 25, 2020

akremsa commented Apr 25, 2020 • edited Loading

agones-bot commented Apr 25, 2020

agones-bot commented Apr 25, 2020

agones-bot commented Apr 25, 2020

markmandel Apr 27, 2020

Choose a reason for hiding this comment

akremsa Apr 27, 2020

Choose a reason for hiding this comment

agones-bot commented Apr 27, 2020

agones-bot commented Apr 27, 2020

google-oss-robot commented Apr 28, 2020

akremsa commented Apr 22, 2020 •

edited

Loading

akremsa Apr 22, 2020 •

edited

Loading

akremsa commented Apr 25, 2020 •

edited

Loading