Fix Integration Tests #129

ben-ng · 2018-04-26T07:05:55Z

Make cluster and container registry configurable via workspace status
Add retries so k8s operations don't flake when they time out
Move test helpers to their own shell script
Add a service.yaml for hellohttp so we can expose it for testing
Add service k8s_object to BUILDs so test deployments are exposed for testing
Fix print statements in hellogrpc servers so assertions work
Clean up sed changes to source files in trap so git is clean after a
run
Remove incomplete todocontroller test from .travis.yml
Update CONTRIBUTING with information on how to run tests

philwo · 2018-05-01T12:50:47Z

.bazelrc

@@ -0,0 +1,2 @@
+build --workspace_status_command=./hack/print-workspace-status.sh


Can you remove this file from the PR?

@philwo hmm but without it, bazel test //... won't work without explicitly specifying that parameter. That doesn't seem user-friendly to me.

+1 this is fine

philwo · 2018-05-01T12:51:11Z

@mattmoor Can you suggest a reviewer for this?

It looks good to me, not sure if a maintainer of rules_k8s wants to have another look.

googlebot · 2018-05-16T14:02:56Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

googlebot · 2018-05-16T14:09:37Z

CLAs look good, thanks!

- Make cluster and container registry configurable via workspace status - Add retries so k8s operations don't flake when they time out - Move test helpers to their own shell script - Add a service.yaml for hellohttp so we can expose it for testing - Add service k8s_object to BUILDs so test deployments are exposed for testing - Fix print statements in hellogrpc servers so assertions work - Clean up sed changes to source files in trap so git is clean after a run - Remove incomplete todocontroller test from .travis.yml - Update CONTRIBUTING with information on how to run tests

ben-ng · 2018-05-16T14:22:19Z

PR updated with a small fix where the e2e test helper wouldn't respect the overridden cluster argument.

Also updated CONTRIBUTING.md with a better way of running all integration tests.

@mattmoor @philwo can we please get this reviewed & merged?

mattmoor

Thanks, some of these best practices sort of popped up after this repo came about, so it's good to see them getting applied. Some comments inline.

mattmoor · 2018-05-18T14:41:17Z

.bazelrc

@@ -0,0 +1,2 @@
+build --workspace_status_command=./hack/print-workspace-status.sh


+1 this is fine

mattmoor · 2018-05-18T14:52:02Z

.travis.yml

-  - ./examples/todocontroller/e2e-test.sh py
+  # Commented out because this test is incomplete and doesn't pass
+  # TODO: Fix this test
+  # - ./examples/todocontroller/e2e-test.sh py


I'd kind of like to see this fixed before this goes in :(

mattmoor · 2018-05-18T14:53:19Z

CONTRIBUTING.md

+
+* A container registry (e.g. [Google Container Registry](https://cloud.google.com/container-registry))
+* A Kubernetes cluster (e.g. [Google Kubernetes Engine](https://cloud.google.com/kubernetes-engine))
+* The host you are running tests from must match the architecture of the Kubernetes nodes (i.e. you cannot run tests on MacOS if your Kubernetes nodes run Linux)


This is only partially true, for Go you can add a flag to .bazelrc that will make everything cross-compile to linux/amd64 and things work fine. For Python/Node this is true :-/

mattmoor · 2018-05-18T14:54:04Z

CONTRIBUTING.md

+(export DOCKER_REPO_OVERRIDE=gcr.io/<your-gcp-project>;
+export BUILD_CLUSTER_OVERRIDE=<your-kubernetes-cluster>;
+grep e2e-test .travis.yml | sed '/^\s*#/ d' | sed 's/^[ -]*//' | \
+while read -r TEST_CMD; do eval "$TEST_CMD"; done)


Perhaps instead, we should add a top-level script that Travis calls :)

mattmoor · 2018-05-18T14:55:01Z

examples/hellogrpc/e2e-test.sh

 function edit() {
   ./examples/hellogrpc/${LANGUAGE}/server/edit.sh "$1"
 }

 function update() {
-   bazel build examples/hellogrpc/${LANGUAGE}/server:staging.replace
-   bazel-bin/examples/hellogrpc/${LANGUAGE}/server/staging.replace


This was to test the .replace flow :(

If todocontroller comes back, we can add it there (no service)

greggdonovan · 2018-05-19T01:18:34Z

CONTRIBUTING.md

+
+**Requirements**
+
+* [Node.js](https://nodejs.org/en/download/).


If we upgrade to a newer version of rules_nodejs can we remove node as a system dependency? The newer version appear to pull in node and yarn in a toolchain-like way.

fejta · 2018-06-26T19:03:49Z

Thanks for the PR! Will you please respond to the feedback thus far and push an update?

fejta · 2018-06-27T04:37:51Z

/cc @mattmoor
/assign

ben-ng · 2018-06-27T04:39:41Z

I'll have time to circle back and update this pull request in a few weeks. We're in the middle of building applications using our fork of rules_k8s, and I don't have the bandwidth to change two things at once.

fejta · 2018-07-18T17:21:37Z

Hi Ben, we replaced travis with prow for running integration tests in CI. Is this still relevant?

ben-ng · 2018-07-18T17:24:37Z

@fejta yes, the integration tests themselves are broken and will not pass without this PR regardless of how you ran them

fejta · 2018-07-18T17:25:59Z

I do not think that is true at head.

#158 passed integration tests: https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/bazelbuild_rules_k8s/158/pull-rules-k8s-e2e/15/

ben-ng · 2018-07-18T17:43:56Z

Oh neat, that's great! It wasn't true when I created this PR.

philwo reviewed May 1, 2018

View reviewed changes

ben-ng force-pushed the configurable-e2e-tests branch from 173359d to 50f909a Compare May 16, 2018 14:02

ben-ng force-pushed the configurable-e2e-tests branch from 50f909a to 5cb53d4 Compare May 16, 2018 14:09

ben-ng force-pushed the configurable-e2e-tests branch from 5cb53d4 to d0e472d Compare May 16, 2018 14:21

mattmoor reviewed May 18, 2018

View reviewed changes

greggdonovan reviewed May 19, 2018

View reviewed changes

fejta added the lifecycle/stale label Jun 26, 2018

k8s-ci-robot assigned fejta Jun 27, 2018

k8s-ci-robot requested a review from mattmoor June 27, 2018 04:37

ben-ng closed this Jul 18, 2018

ben-ng deleted the configurable-e2e-tests branch July 18, 2018 17:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Integration Tests #129

Fix Integration Tests #129

ben-ng commented Apr 26, 2018

philwo May 1, 2018

ben-ng May 7, 2018

mattmoor May 18, 2018

philwo commented May 1, 2018

googlebot commented May 16, 2018

googlebot commented May 16, 2018

ben-ng commented May 16, 2018

mattmoor left a comment

mattmoor May 18, 2018

mattmoor May 18, 2018

mattmoor May 18, 2018

mattmoor May 18, 2018

mattmoor May 18, 2018

mattmoor May 18, 2018

greggdonovan May 19, 2018

fejta commented Jun 26, 2018

fejta commented Jun 27, 2018

ben-ng commented Jun 27, 2018

fejta commented Jul 18, 2018

ben-ng commented Jul 18, 2018

fejta commented Jul 18, 2018

ben-ng commented Jul 18, 2018

		@@ -0,0 +1,2 @@
		build --workspace_status_command=./hack/print-workspace-status.sh


		Requirements

		* [Node.js](https://nodejs.org/en/download/).

Fix Integration Tests #129

Fix Integration Tests #129

Conversation

ben-ng commented Apr 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philwo commented May 1, 2018

googlebot commented May 16, 2018

googlebot commented May 16, 2018

ben-ng commented May 16, 2018

mattmoor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fejta commented Jun 26, 2018

fejta commented Jun 27, 2018

ben-ng commented Jun 27, 2018

fejta commented Jul 18, 2018

ben-ng commented Jul 18, 2018

fejta commented Jul 18, 2018

ben-ng commented Jul 18, 2018