Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improved Fleets - controller tests #1547

Merged
merged 5 commits into from
Jun 8, 2020
Merged

Improved Fleets - controller tests #1547

merged 5 commits into from
Jun 8, 2020

Conversation

akremsa
Copy link
Contributor

@akremsa akremsa commented May 14, 2020

What type of PR is this?

/kind cleanup

What this PR does / Why we need it:
Added more tests to fleets - controller_test.go

Special notes for your reviewer:
BEFORE:
image

AFTER:
image
image

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: e7ad7ca8-bc06-4257-9313-998a8e50ea45

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

aLekSer
aLekSer reviewed May 14, 2020
View reviewed changes
pkg/fleets/controller_test.go

gsSet := f.GameServerSet()
// make gsSet.Spec.Template and f.Spec.Template different in order to make 'rest' list not empty
gsSet.Spec.Template.ClusterName = "qqqqqqqqqqqqqqqqqqq"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
gsSet.Spec.Template.ClusterName = "qqqqqqqqqqqqqqqqqqq"
gsSet.Spec.Template.ClusterName = strings.Repeat("q", 19)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see it is not resolved

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use a const for this and above?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's not resolved because I prefer to keep it as is. Honestly, I don't see any difference between a pure string and strings.Repeat(). In my implementation, it's easier to notice that this is an invalid value.
According to constants - I prefer to avoid them in tests and this is just a random "invalid" sequence of characters. This is not that type of stuff which is worth much attention, in my opinion.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it might be not crucial here. Just it looks nicer.

@akremsa akremsa force-pushed the fleet-controller-tests branch from bf1e487 to eabeb25 Compare May 15, 2020 12:33
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: b9050cbd-26ea-414c-b592-889f54778958

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:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1547/head:pr_1547 && git checkout pr_1547
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.6.0-eabeb25

@akremsa akremsa force-pushed the fleet-controller-tests branch from eabeb25 to 7ee5c1f Compare May 18, 2020 09:52
@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 306ea29a-8241-4e24-9ea9-5c743b47f36d

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

@akremsa akremsa force-pushed the fleet-controller-tests branch from 7ee5c1f to 23d72ef Compare May 19, 2020 12:17
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 9bf1055f-6eb1-47f0-924f-2d9160fcc95e

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:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1547/head:pr_1547 && git checkout pr_1547
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.6.0-23d72ef

@akremsa akremsa force-pushed the fleet-controller-tests branch from 23d72ef to 3a52b1d Compare May 22, 2020 12:28
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: bf964757-57b7-4be0-a3e5-05c5dc1ce0c8

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:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1547/head:pr_1547 && git checkout pr_1547
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.6.0-3a52b1d

@akremsa akremsa force-pushed the fleet-controller-tests branch 2 times, most recently from 5156ef4 to a100d3f Compare June 1, 2020 11:37
@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 637711b3-52fe-4c26-a2dc-9cb9cdb90b80

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

markmandel
markmandel reviewed Jun 1, 2020
View reviewed changes
pkg/fleets/controller.go
@@ -408,10 +408,10 @@ func (c *Controller) recreateDeployment(fleet *agonesv1.Fleet, rest []*agonesv1.
func (c *Controller) rollingUpdateDeployment(fleet *agonesv1.Fleet, active *agonesv1.GameServerSet, rest []*agonesv1.GameServerSet) (int32, error) {
replicas, err := c.rollingUpdateActive(fleet, active, rest)
if err != nil {
return replicas, err
return 0, err
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious, what's the impetus for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left a comment here
#1531 (comment)

pkg/fleets/controller_test.go Outdated Show resolved Hide resolved
pkg/fleets/controller_test.go Outdated Show resolved Hide resolved
@akremsa akremsa force-pushed the fleet-controller-tests branch from a100d3f to 0932d24 Compare June 2, 2020 21:01
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: fd0864a8-ed18-4b8a-b503-3a57cbe9b143

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:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1547/head:pr_1547 && git checkout pr_1547
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.7.0-0932d24

@akremsa akremsa force-pushed the fleet-controller-tests branch from 0932d24 to 5fe60db Compare June 4, 2020 23:26
aLekSer
aLekSer suggested changes Jun 5, 2020
View reviewed changes
Copy link
Collaborator

@aLekSer aLekSer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments. Overall looks good.

@alexey-kremsa-globant
Added more tests
ca9e787 
TestControllerCreationMutationHandler, TestControllerCreationValidationHandler, TestControllerUpsertGameServerSet
more tests


more tests


review notes
@akremsa akremsa force-pushed the fleet-controller-tests branch from 5fe60db to c221f96 Compare June 8, 2020 11:08
@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 4bfd14dc-5e58-4dcc-bb43-d4b7fd76b0b5

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

@akremsa akremsa force-pushed the fleet-controller-tests branch from c221f96 to 60b12dc Compare June 8, 2020 12:42
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 53686a4f-4717-4abf-8724-d986b53f7ac4

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:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1547/head:pr_1547 && git checkout pr_1547
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.7.0-60b12dc

@akremsa akremsa requested a review from aLekSer June 8, 2020 13:05
@akremsa
Copy link
Contributor Author

akremsa commented Jun 8, 2020

@aLekSer @markmandel
I've updated PR, please check again.

@pooneh-m pooneh-m removed their request for review June 8, 2020 13:16
aLekSer
aLekSer reviewed Jun 8, 2020
View reviewed changes
Copy link
Collaborator

@aLekSer aLekSer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updates

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: akremsa, aLekSer
To complete the pull request process, please assign pooneh-m
You can assign the PR to them by writing /assign @pooneh-m in a comment when ready.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-robot
Copy link

New changes are detected. LGTM label has been removed.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: e65426df-fa6e-4391-8dae-34789df566fa

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:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1547/head:pr_1547 && git checkout pr_1547
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.7.0-b19e4d4

@aLekSer
Copy link
Collaborator

aLekSer commented Jun 8, 2020

@markmandel I think this PR is ready, can you please look one more time?

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 638ab1bf-ab55-4293-82db-1b5fd67003bb

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:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1547/head:pr_1547 && git checkout pr_1547
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.7.0-769c8ae

@markmandel markmandel merged commit 6475404 into googleforgames:master Jun 8, 2020
@markmandel 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 8, 2020
@markmandel markmandel added this to the 1.7.0 milestone Jun 8, 2020
ilkercelikyilmaz pushed a commit to ilkercelikyilmaz/agones that referenced this pull request Oct 23, 2020
@alexey-kremsa-globant @aLekSer @ilkercelikyilmaz
Improved Fleets - controller tests (googleforgames#1547)
c4b9e02 
* added syncFleet tests

Co-authored-by: Alexander Apalikov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markmandel markmandel markmandel left review comments

@aLekSer aLekSer aLekSer approved these changes

@cyriltovena cyriltovena Awaiting requested review from cyriltovena

Assignees

@aLekSer aLekSer

Labels
area/tests Unit tests, e2e tests, anything to make sure things don't break kind/cleanup Refactoring code, fixing up documentation, etc size/XL
Projects
None yet
Milestone
1.7.0
Development

Successfully merging this pull request may close these issues.
6 participants
@akremsa @agones-bot @google-oss-robot @aLekSer @markmandel @alexey-kremsa-globant