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.

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

Add flags which allow to pass namespace to e2e tests #1623

Merged
merged 5 commits into from
Jun 18, 2020

Conversation

akremsa
Copy link
Contributor

@akremsa akremsa commented Jun 11, 2020

What type of PR is this?

/kind feature

What this PR does / Why we need it:
We need this flag in order to be able to pass a randomly generated namespace to e2e tests.

Which issue(s) this PR fixes:
Closes #1074

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: d96a0a39-6643-48bf-86ea-cfc7d47b15b7

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

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 65035d33-96de-45c8-9319-6e06612cfa38

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/1623/head:pr_1623 && git checkout pr_1623
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.7.0-23ddf83

@akremsa akremsa changed the title Added flags which allow to pass namespace to e2e tests Add flags which allow to pass namespace to e2e tests Jun 11, 2020
@pooneh-m
Copy link
Contributor

How about creating a new namespace at the test runtime?

@aLekSer
Copy link
Collaborator

aLekSer commented Jun 11, 2020

May be we can use both options, a flag to generate the namespace and another option as in this PR. Do you mean we should add ServiceAccount and ClusterRole per gameservers.namespaces not in make install step but in the test?

@pooneh-m
Copy link
Contributor

pooneh-m commented Jun 11, 2020

May be we can use both options, a flag to generate the namespace and another option as in this PR. Do you mean we should add ServiceAccount and ClusterRole per gameservers.namespaces not in make install step but in the test?

CreateNamespace() also sets up a new namespaces with ServiceAccount but for test only. If your motivation to add the flag is for testing on a non-default namespace, using CreateNamespace() does that for you, with all the required setup.

@pooneh-m
Copy link
Contributor

The original idea for #1074 was to create a namespace per test suite at runtime and use the new namespace instead of default to help with isolation and debugging e.g. here. Is there a usecase for passing in a namespace as an argument?

@aLekSer
Copy link
Collaborator

aLekSer commented Jun 12, 2020

@pooneh-m, now I got your idea.
I think for backward compatibility, let's leave an option to use default or any other namespace. If namespace is an empty string then use random approach you are suggesting.
So running:
make test-e2e GAMESERVERS_NAMESPACE=default
is equivalent to current state.
make test-e2e new randomised approach.
createNamespace(time.Now()) could be used for example.
This way we can switch back easily if something goes wrong.
And no changes to e2e.sh is needed in Cloud Build.
By the way CreateNamespace() should be renamed to CreateNamespaceWithServiceAccount() or just include this into comment in:
https://github.com/googleforgames/agones/blob/master/test/e2e/framework/framework.go#L490

@akremsa
Copy link
Contributor Author

akremsa commented Jun 12, 2020

I will update this PR

@akremsa
Copy link
Contributor Author

akremsa commented Jun 16, 2020

@pooneh-m @aLekSer please, review this PR again.

At this moment there is the following behavior:
GAMESERVERS_NAMESPACE is empty by default, so random namespace will be created after you run make test-e2e.
If you specify a namespace like make test-e2e GAMESERVERS_NAMESPACE="xbox", agones will run tests against specified namespace. If there is no such a namespace - error will be returned. In this case, you need to create a namespace beforehand.

One remark: I've noticed that in case if an error is returned after namespace is created and tests fail with error - that recently created namespace will remain. Should I add better error validations to remove dangling namespaces or it's better to do that in a separate PR?

@aLekSer
Copy link
Collaborator

aLekSer commented Jun 16, 2020

Overall looks good, but I think we should give some hints how to run test-e2e with and without GAMESERVERS_NAMESPACE flag in build/README.md and probably additional comment in Makefile itself is needed.

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.

Minor comments left about additional details on flags usage.

@aLekSer
Copy link
Collaborator

aLekSer commented Jun 16, 2020

E2E test failed:

time="2020-06-16 13:58:19.721" level=info msg="failing Allocate request" error="rpc error: code = Unavailable desc = all SubConns are in TransientFailure, latest connection error: connection error: desc = \"transport: authentication handshake failed: x509: cannot validate certificate for 35.233.192.59 because it doesn't contain any IP SANs\""
--- FAIL: TestTlsWebhook (391.55s)
    fleetautoscaler_test.go:636: error waiting for fleet condition on fleet simple-fleet-778hp
FAIL
time="2020-06-16 14:05:07.569" level=info msg="Namespace 1592315887 is deleted"

@pooneh-m
Copy link
Contributor

pooneh-m commented Jun 16, 2020

@pooneh-m @aLekSer please, review this PR again.

Looks great!!! Thanks for the change.

At this moment there is the following behavior:
GAMESERVERS_NAMESPACE is empty by default, so random namespace will be created after you run make test-e2e.
If you specify a namespace like make test-e2e GAMESERVERS_NAMESPACE="xbox", agones will run tests against specified namespace. If there is no such a namespace - error will be returned. In this case, you need to create a namespace beforehand.

One remark: I've noticed that in case if an error is returned after namespace is created and tests fail with error - that recently created namespace will remain. Should I add better error validations to remove dangling namespaces or it's better to do that in a separate PR?

The defer should clean up the namespace, unless there is a failure with deleting the namespace. If the namespace is pre-created, then the creator of the namespace should clean up.

test/e2e/main_test.go Outdated Show resolved Hide resolved
test/e2e/main_test.go Outdated Show resolved Hide resolved
test/e2e/main_test.go Outdated Show resolved Hide resolved
@akremsa
Copy link
Contributor Author

akremsa commented Jun 17, 2020

@aLekSer thanks for comments. Updated.

@google-oss-robot
Copy link

New changes are detected. LGTM label has been removed.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 58f771ac-fef4-4e49-803b-4d677a62a5bf

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

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 9f50c457-e8a8-4459-a463-4b78dd32648e

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/googleforgames/agones.git pull/1623/head:pr_1623 && git checkout pr_1623
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.7.0-a1e7b21

test/e2e/allocator_test.go Outdated Show resolved Hide resolved
test/e2e/allocator_test.go Outdated Show resolved Hide resolved
@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 63d01592-d64e-45f2-9d6d-3673cf58b56e

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

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 03887d1b-9a3e-4707-bfc1-cc6ba2296808

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

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: ea19c2cc-f01d-4d71-8a86-c8c48b5a5686

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

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 5ccd4fc5-16f5-402f-86cd-1e93393506d4

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

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 322c2409-8231-465b-9087-8f15c45e947d

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/googleforgames/agones.git pull/1623/head:pr_1623 && git checkout pr_1623
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.7.0-9dd28ad

@pooneh-m pooneh-m merged commit ee2f9b1 into googleforgames:master Jun 18, 2020
@markmandel markmandel added this to the 1.7.0 milestone Jun 30, 2020
@markmandel markmandel added area/tests Unit tests, e2e tests, anything to make sure things don't break kind/feature New features for Agones labels Jun 30, 2020
ilkercelikyilmaz pushed a commit to ilkercelikyilmaz/agones that referenced this pull request Oct 23, 2020
…1623)

* Added flags which allow to pass namespace to e2e tests

* use random namespace when flag is empty


namespace flag is empty by default

* Added a couple of comments related to GAMESERVERS_NAMESPACE flag

* Fixed TestTlsWebhook test

* reuse existing namespace in several tests

update
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved area/tests Unit tests, e2e tests, anything to make sure things don't break cla: yes kind/feature New features for Agones size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

E2E tests should use a randomly created Namespace for testing
9 participants