Features/e2e #315
Merged
Features/e2e #315
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Build Failed 😱 Build Id: 345b7c27-67d7-45a8-b126-aa77a4d274f3 Build Logs |
|
Build Failed 😱 Build Id: ef162a45-4b9d-4217-a0f2-d8afcc690f98 Build Logs |
|
Build Succeeded 👏 Build Id: d5c68fdc-5183-459e-bc13-f235ef82606f The following development artifacts have been built, and will exist for the next 30 days:
|
markmandel
reviewed
Aug 2, 2018
| @@ -85,6 +88,10 @@ include ./includes/$(osinclude) | |||
| # personal includes, excluded from the git repository | |||
| -include ./local-includes/*.mk | |||
|
|
|||
| ifdef DOCKER_RUN | |||
There was a problem hiding this comment.
So I'm curious - what does this do, and why is it necessary?
There was a problem hiding this comment.
I couldn't get our docker run type of target get the gcloud service account of the cloud builder to access our kubernetes cluster, so I ended up using a custom 2e2 image which is a FROM gcr.io/cloud-builders/gcloud and has a gcloud already configured automagically :squirrel:.
Since I have run without docker in the build system, I need to call the make target without doing a docker run and calling the go test command straight. So this line make sure I don't run ensure-build-image if DOCKER_RUN is empty, which otherwise would fail.
I wanted to keep the runner using our make targets (install and test-2e2) so we don't end up with too many targets. Another option would be to have special targets only for the runner and we would replicate the go test and the helm install command.
I'm ok to shift toward a multiple makefile system if you think that is clearer, let me know.
There was a problem hiding this comment.
SGTM. I (think) I follow.
I can't think of a better way to do this 😄 so yeah, let's run with it for now 👍
build/build-image/Dockerfile
Outdated
| @@ -87,6 +92,19 @@ RUN wget -q https://static.rust-lang.org/rustup/archive/1.11.0/${RUST_ARCH}/rust | |||
| RUN cargo install protobuf-codegen --vers 2.0.2 | |||
| RUN cargo install grpcio-compiler --vers 0.3.0 | |||
|
|
|||
| # set up Consul. | |||
There was a problem hiding this comment.
I take it we need consul locally to run e2e against local clusters?
There was a problem hiding this comment.
No we don’t, I left it in case we would need it for testing but I’m happy to remove it, WDYT ?
There was a problem hiding this comment.
I'd say let's remove it - it's an added step we have then have to maintain, and keep up to date.
test/e2e/framework/framework.go
Outdated
| var pollErr error | ||
| var readyGs *v1alpha1.GameServer | ||
|
|
||
| err := wait.Poll(2*time.Second, timeout, func() (bool, error) { |
There was a problem hiding this comment.
Should we Poll or PollImmediate ? If we Poll we always wait for 2 second, when maybe we don't need to?
There was a problem hiding this comment.
yes you're right ! let me change that
test/e2e/gameserver_test.go
Outdated
| "testing" | ||
|
|
||
| "agones.dev/agones/pkg/apis/stable/v1alpha1" | ||
| agonesFramework "agones.dev/agones/test/e2e/framework" |
There was a problem hiding this comment.
Any reason we can't use "framework"?
Pretty sure it's not canonical go to use camelCase here. @enocom I'll defer to you?
Looking at https://golang.org/doc/effective_go.html#package-names
"By convention, packages are given lower case, single-word names; there should be no need for underscores or mixedCaps"
"Another convention is that the package name is the base name of its source directory; the package in src/encoding/base64 is imported as "encoding/base64" but has name base64, not encoding_base64 and not encodingBase64."
But I couldn't find anything specific about import aliases.
There was a problem hiding this comment.
Yep, @markmandel, you're right. Named imports should be all lowercase.
There was a problem hiding this comment.
framework is used by a global variable, do you think I should not use a global variable ? I could renamed that import to e2eframework ?
| t.Fatalf("Could ping GameServer: %v", err) | ||
| } | ||
|
|
||
| assert.Equal(t, reply, "ACK: Hello World !\n") |
There was a problem hiding this comment.
👯♀️ 👯♀️ 👯♀️ 👯♀️ 👯♀️ 👯♀️ 👯♀️
build/e2e-image/entrypoint.sh
Outdated
| @@ -0,0 +1,35 @@ | |||
| #!/usr/bin/env bash | |||
|
|
|||
| # Copyright 2017 Google Inc. All Rights Reserved. | |||
build/e2e-image/e2e.sh
Outdated
| @@ -0,0 +1,19 @@ | |||
| #!/usr/bin/env bash | |||
|
|
|||
| # Copyright 2017 Google Inc. All Rights Reserved. | |||
| @@ -0,0 +1,44 @@ | |||
| # Copyright 2017 Google Inc. All rights reserved. | |||
| @@ -0,0 +1,16 @@ | |||
| # E2E Testing | |||
There was a problem hiding this comment.
Should we add a section to our Development guide on this - at least we should add the new major Make targets?
I feel like there should be a "how to run e2e tests" section, or maybe section per deployment? Or something.
Then I can follow the guide, and make sure it all works 😄
There was a problem hiding this comment.
yes but I think only make e2e would be sufficient no ? Do you see a need to talk about the e2e cluster ? Since people could just use their development cluster or minikube which is already well documented.
So far how to run e2e is pretty straightforward after you've setup your cluster, just run make test-e2e. I can add small sections about how just after talking about Running a Test Minikube cluster which would explain how to run against those 2 clusters and include the make target.
Do you think I should explain the e2e image and gcloud-e2e targets which are exclusively for cloud builder, I'm actually not sure that's why I ask ?
There was a problem hiding this comment.
No, I think you are right - we should document how to run the e2e tests, I don't think we need to document how they are built.
Will need to also have make minikube-test-e2e in the minikube section but that's about it.
👍
There was a problem hiding this comment.
I've updated the build/ documentation, let me know.
test/e2e/README.md
Outdated
| what to run, timeout length. Let's say I want to run all tests in "test/e2e/": | ||
|
|
||
| ``` | ||
| $ go test -v ./test/e2e/ --kubeconfig "$HOME/.kube/config" --gameserver-image=gcr.io/agones-images/cpp-simple-server:0.2 |
There was a problem hiding this comment.
Should we be running tests this way, or through make e2e ?
There was a problem hiding this comment.
you should use make e2e, I will change.
markmandel
reviewed
Aug 2, 2018
build/Makefile
Outdated
| $(agones_package)/sdks/... | ||
|
|
||
| # Runs end-to-end tests on the current configured cluster | ||
| test-e2e: |
There was a problem hiding this comment.
We'll also need a minikube-test-e2e target, just because it has special mounting for .kube config and should run on host network. Should be able to lift an existing example, or I can write it if you want.
(Just tried it against minikube, and it didn't work - but that's not surprising)
There was a problem hiding this comment.
let me see if I can get it working.
|
Build Failed 😱 Build Id: b5cbe5be-7bc4-4878-9c20-e3dfc6a08ebf Build Logs |
cyriltovena
force-pushed
the
features/e2e
branch
from
77ff848 to
d4b1cf5
Compare
|
Build Failed 😱 Build Id: aa2af5c0-2b6b-4850-9a0a-72987c1e9f7a Build Logs |
cyriltovena
force-pushed
the
features/e2e
branch
from
d4b1cf5 to
5d4a38f
Compare
|
Build Succeeded 👏 Build Id: aea83784-0413-4075-8214-03dde74b5ade The following development artifacts have been built, and will exist for the next 30 days:
|
|
Can also confirm that minikube test worked! ➜ build git:(pr_315) make minikube-test-e2e
minikube profile default
minikube profile was successfully set to minikube
docker run --rm -v /home/markmandel/workspace/agones/src/agones.dev/agones/build//.config/gcloud:/root/.config/gcloud -v ~/.kube:/root/.kube -v /home/markmandel/workspace/agones/src/agones.dev/agones:/go/src/agones.dev/agones --network=host -v ~/.minikube:/home/markmandel/.minikube agones-build:6a4e0e6230 go test -v agones.dev/agones/test/e2e/... \
--kubeconfig /root/.kube/config \
--gameserver-image=gcr.io/agones-images/cpp-simple-server:0.2
=== RUN TestCreateConnect
=== PAUSE TestCreateConnect
=== CONT TestCreateConnect
--- PASS: TestCreateConnect (6.09s)
PASS
ok agones.dev/agones/test/e2e 6.213s
? agones.dev/agones/test/e2e/framework [no test files]
➜ build git:(pr_315) |
markmandel
added
area/build-tools
cyriltovena
force-pushed
the
features/e2e
branch
from
5d4a38f to
d753d3d
Compare
|
Build Failed 😱 Build Id: e8fd7db5-7926-450e-8a95-3ec997a6290f Build Logs |
cyriltovena
force-pushed
the
features/e2e
branch
from
d753d3d to
dc0e9dd
Compare
|
Build Succeeded 👏 Build Id: eea308eb-e12b-4cca-abef-46be0a31400d The following development artifacts have been built, and will exist for the next 30 days:
(experimental) To install this version:
|
markmandel
reviewed
Aug 16, 2018
build/build-image/Dockerfile
Outdated
| RUN gcloud components update | ||
|
|
||
| # install kubectl without gcloud as we want the last version | ||
| ENV KUBECTL_VER 1.11.0 |
There was a problem hiding this comment.
Super minor thing. Do we want 1.10? I don't think you can run 1.11 on anything yet?
There was a problem hiding this comment.
Good question ! I was lazy to figured which version I need.
I updated kubectl because the version from gcloud doesn't support port-forward on stateful set and I needed it there :
it has been introduced by kubernetes/kubernetes#59705 which seems to be 1.10, do you want me to downgrade ?
There was a problem hiding this comment.
I think so - it's super minor, bit it's nice to have our supported version line up with our Dev tooling, Just to make sure nothing odd happens.
Other than that - swash to a single commit, and I'll got approve!
| @@ -0,0 +1,16 @@ | |||
| # E2E Testing | |||
cyriltovena
force-pushed
the
features/e2e
branch
from
dc0e9dd to
ac28dcb
Compare
This test creates a gameserver and connect to it update vendor with required lib for e2e Adds the gcloud builder step to test it in a special e2e cluster Updates build documentations with e2e instructions Adds minikube e2e target
cyriltovena
force-pushed
the
features/e2e
branch
from
ac28dcb to
3662c17
Compare
|
Build Succeeded 👏 Build Id: c64e623f-ec33-4d46-9782-356c8afde787 The following development artifacts have been built, and will exist for the next 30 days:
(experimental) To install this version:
|
|
Build Succeeded 👏 Build Id: 606e4423-f417-46d1-bf23-cd6105314d7c The following development artifacts have been built, and will exist for the next 30 days:
(experimental) To install this version:
|
markmandel
approved these changes
Aug 16, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a sample of one e2e tests for #37 . This first test create a gameserver, waits for readiness, assert some values then finally ping it (send/receive).
Everything is plugged to cloud builder and run against a real gke cluster, branch will run sequentially using a consul lock within the cluster.
Flow of the runner is :
That said there is a target if you want to run the e2e against your cluster (minikube/gke/....) from your machine.
The idea of this PR is to see if the direction is correct.
Ticket #37 has more details of remaining work to do.
Have a spin, I hope you like it.
PS: this also fixes #173
e2e runner sample output