No test framework #778

bobcatfish · 2018-04-29T18:59:00Z

As explored in the prototypes in #253, we have decided that although Ginkgo is the best of the Go test frameworks, we do not want to be tied to BDD and feel these tests will be cleaner and easier to contribute to if we use no framework.

I have also refactored the supporting logic of the tests into a test library which can be reused for other e2e tests, documented in test/adding_tests.md

Improved docs on running tests: now all docs on running are in test/README.md, there is only one place to look to see how to run all of the tests. Instructions on how to run conformance tests are hopefully more clear.
Updated links that pointed at CONTRIBUTING.md since the content moved

Fixes #253

Proposed Changes

Remove Ginkgo framework
All instructions for running tests are in test/README.md
Add test library for use in e2e tests, documented in test/adding_tests.md

Release Note

NONE

There are two commits, one to make the changes and the other to update deps, etc.

😎

This is what the conformance tests look like when they run:

bobcatfish · 2018-04-29T22:16:07Z

/retest

bobcatfish · 2018-04-29T23:58:04Z

Hm weird errror

I0429 22:58:31.131] pizzaplanet    1m          2m           2         prod-00001-deployment-68d568ddfb-22x4t.152a0a27f650c33f                      Pod             spec.containers{ela-container}               Warning   Failed                    kubelet, gke-ela-e2e-cluster-default-pool-4973f363-s3z5      Failed to pull image "gcr.io/gke-gerrit-boskos-03/pizzaplanetv1": rpc error: code = Unknown desc = Error: Status 405 trying to pull repository gke-gerrit-boskos-03/pizzaplanetv1: "v1 Registry API is disabled. If you are not explicitly using the v1 Registry API, it is possible your v2 image could not be found. Verify that your image is available, or retry with `dockerd --disable-legacy-registry`. See https://cloud.google.com/container-registry/docs/support/deprecation-notices"
I0429 22:58:31.131] pizzaplanet    1m          2m           3         prod-00001-deployment-68d568ddfb-22x4t.152a0a27f651487c                      Pod             spec.containers{ela-container}               Warning   Failed                    kubelet, gke-ela-e2e-cluster-default-pool-4973f363-s3z5      Error: ErrImagePull

/retest

bobcatfish · 2018-04-30T00:59:47Z

Ah okay, I wasn't handling the docker repo argument correctly.

adrcunha

Initial review...

adrcunha · 2018-04-30T14:50:00Z

test/README.md

+* Provide a docker repo from which the built images wil be pulled. This is done
+  via the `--dockerrepo` argument.
+
+_The `bazel` execution environment will not contain your enviornment variables, so you must


typo: environment

Also, type wil->will

adrcunha · 2018-04-30T14:51:04Z

test/README.md

+
+To run the conformance tests with `bazel` you must:
+
+* Provide a `kubeconfig` file that is a `data` dependency of the test


Isn't there a period after "test"? I read this sentence 3 times and although I understand it technically, the wording sounds confusing to me...

Period makes it clearer! I'll also break this into a couple of sentences to try to make it easier to understand.

adrcunha · 2018-04-30T14:53:17Z

test/README.md


-The end-to-end tests can be run by executing `e2e-tests.sh --run-tests` in the command line.
+These flags useful for running against an existing cluster, making use of your existing


"flags are useful"?

adrcunha · 2018-04-30T14:55:20Z

test/README.md

+To run the conformance tests with a non-default kubeconfig file:
+
+```bash
+go test ./test/conformance -args --kubeconfig /my/path/kubeconfig


Why "-args" here, but not in the examples before?

ah thanks for catching this - when i realized the dockerrepo arg wasn't working a i got confused and thought I needed to provide -args before passing test arguments, but apparently -args does something completely different than I thought it did:

To keep an argument for a test binary from being interpreted as a
known flag or a package name, use -args (see 'go help test') which
passes the remainder of the command line through to the test binary
uninterpreted and unaltered.

Will remove -args from the docs!

adrcunha · 2018-04-30T14:55:27Z

test/README.md

+if not specified.
+
+```bash
+go test ./test/conformance -args --cluster your-cluster-name


Why "-args" here, but not in the examples before?

adrcunha · 2018-04-30T14:56:09Z

test/README.md

+go test ./test/conformance -args --cluster your-cluster-name
+```
+
+The current cluster names can be obtained by runing:


typo: running

adrcunha · 2018-04-30T14:56:40Z

test/README.md

+if not specified.
+
+```bash
+go test ./test/conformance -args --dockerrepo gcr.myhappyproject


Why "-args" here, but not in the examples before?

adrcunha · 2018-04-30T15:00:18Z

@nikkithurmond Christie's library should make it easier rewriting the autoscaler tests in go. 😄

As explored in the prototypes in #253, we have decided that although Ginkgo is the best of the Go test frameworks, we do not want to be tied to BDD and feel these tests will be cleaner and easier to contribute to if we use no framework. I have also refactored the supporting logic of the tests into a `test` library which can be reused for other e2e tests, documented in test/adding_tests.md * Improved docs on running tests: now all docs on running are in test/README.md, there is only one place to look to see how to run all of the tests. Instructions on how to run conformance tests are hopefully more clear. * Updated links that pointed at CONTRIBUTING.md since the content moved Fixes #253

Removed ginkgo + gomega.

bobcatfish · 2018-04-30T17:38:49Z

@adrcunha initial issues should be addressed now!

adrcunha · 2018-04-30T19:27:47Z

Suggestion: move the library to its own subdirectory inside test.

adrcunha · 2018-04-30T19:28:29Z

As discussed earlier, let's merge this PR so Nicole can make use of the library for the autoscaler E2E tests in go; feedback can be addressed in a future PR.

adrcunha · 2018-04-30T19:28:36Z

/lgtm
/approve

google-prow-robot · 2018-04-30T19:28:43Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: adrcunha, bobcatfish
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: vaikas-google

Assign the PR to them by writing /assign @vaikas-google in a comment when ready.

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

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

bobcatfish · 2018-04-30T22:15:24Z

Suggestion: move the library to its own subdirectory inside test.

I like this idea and I think the test/ dir would be cleaner. Also then instead of adding_tests.md I could make a README in that dir.

But on the other hand I think it makes sense that the package is called test and if I made a subdir, I can't think of what to call it 🤔 The only ideas that come to mind are util or lib but I also think those are bad package names. The next best thing I can think of is test/test, so the package would still be called test. Any naming ideas @adrcunha ?

bobcatfish · 2018-04-30T22:16:45Z

I need a /lgtm from @vaikas-google @mattmoor or @evankanderson b/c of the changes to DEVELOPMENT.md and b/c I'm removing dependencies from vendor/.

dprotaso · 2018-05-01T01:01:27Z

FYI for @dprotaso once this merges - we'll need to update the conformance test in #786

adrcunha · 2018-05-01T01:10:41Z

But on the other hand I think it makes sense that the package is called test and if I made a subdir, I can't > think of what to call it 🤔 The only ideas that come to mind are util or lib but I also think those are bad
package names. The next best thing I can think of is test/test, so the package would still be called test.

framework? Or we could have //test/e2e for the tests themselves, and //test/e2e/framework for the framework? (or //test/e2e/lib)

bobcatfish · 2018-05-01T15:29:25Z

framework? Or we could have //test/e2e for the tests themselves, and //test/e2e/framework for the framework? (or //test/e2e/lib)

One thing I like about the current libs are that they are just libs, and not yet a framework (i.e. they don't require anything about what is called when, or what the entry point is, things a framework would be very prescriptive about).

or //test/e2e/lib

I could go with lib if we can't think of anything better - I feel like lib is too generic to provide any information, like in the go package name recommendations:

Packages named util, common, or misc provide clients with no sense of what the package contains. This makes it harder for clients to use the package and makes it harder for maintainers to keep the package focused.

But maybe in the context of the test dir, lib is meaningful, b/c it's distinguishing the libs from the tests themselves? A middle ground would be testlib. Or elafrostest?

Or we could have //test/e2e for the tests themselves,

I like that! I think it makes sense to have an e2e dir, like the conformance dir

bobcatfish · 2018-05-01T15:31:54Z

@vaikas-google @mattmoor and @evankanderson I'm going to cheat and merge this without tide b/c I think you may be traveling and so @nikkithurmond and others can make use of / react to the changes.

If you notice anything you want changed tho plz point it out and I'll open another PR.

…1428) In #475 I removed assertions against Configuration.Spec.Generation b/c it is a hack and not part of the knative spec. In #600 I updated the conformance tests to assert against the Revision annotation which contains the generation. BUT THEN in #778 when I completely re-wrote the tests to no longer use Ginkgo, I accidentally undid both of those changes, so this commit puts them back 😅. BONUS: I hit a case where the length of the loadbalancer ingresses was 0 and got a panic, so if that happens again we'll get an informative error instead.

bobcatfish requested review from grantr, tcnghia, vaikas and adrcunha April 29, 2018 18:59

bobcatfish requested review from srinivashegde86 and steuhs as code owners April 29, 2018 18:59

google-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Apr 29, 2018

bobcatfish changed the title ~~No framework~~ No test framework Apr 29, 2018

bobcatfish force-pushed the no_framework branch 2 times, most recently from 9ef9336 to 8bd9aa3 Compare April 29, 2018 22:44

bobcatfish force-pushed the no_framework branch from 8bd9aa3 to 2b57d3e Compare April 30, 2018 00:59

bobcatfish force-pushed the no_framework branch from 2b57d3e to 656b69b Compare April 30, 2018 03:13

adrcunha suggested changes Apr 30, 2018

View reviewed changes

bobcatfish added 2 commits April 30, 2018 10:37

Run hack/update-deps.sh

0391432

Removed ginkgo + gomega.

bobcatfish force-pushed the no_framework branch from 656b69b to 0391432 Compare April 30, 2018 17:38

google-prow-robot assigned adrcunha Apr 30, 2018

google-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 30, 2018

adrcunha approved these changes Apr 30, 2018

View reviewed changes

bobcatfish requested review from mattmoor and evankanderson April 30, 2018 22:17

bobcatfish merged commit c2c0120 into knative:master May 1, 2018

bobcatfish mentioned this pull request Jun 29, 2018

Assert against Revision generation annotation instead of config spec #1428

Merged

skonto pushed a commit to skonto/serving that referenced this pull request Jun 3, 2021

Drop now unnecessary dryrun patch (knative#778)

f4389f4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

No test framework #778

No test framework #778

bobcatfish commented Apr 29, 2018

bobcatfish commented Apr 29, 2018

bobcatfish commented Apr 29, 2018

bobcatfish commented Apr 30, 2018

adrcunha left a comment

adrcunha Apr 30, 2018

nikkithurmond Apr 30, 2018

bobcatfish Apr 30, 2018

adrcunha Apr 30, 2018

bobcatfish Apr 30, 2018

adrcunha Apr 30, 2018

bobcatfish Apr 30, 2018

adrcunha Apr 30, 2018

bobcatfish Apr 30, 2018

adrcunha Apr 30, 2018

adrcunha Apr 30, 2018

adrcunha Apr 30, 2018

adrcunha commented Apr 30, 2018 •

edited

Loading

bobcatfish commented Apr 30, 2018

adrcunha commented Apr 30, 2018

adrcunha commented Apr 30, 2018

adrcunha commented Apr 30, 2018

google-prow-robot commented Apr 30, 2018

bobcatfish commented Apr 30, 2018

bobcatfish commented Apr 30, 2018 •

edited

Loading

dprotaso commented May 1, 2018

adrcunha commented May 1, 2018

bobcatfish commented May 1, 2018

bobcatfish commented May 1, 2018


		To run the conformance tests with `bazel` you must:

		* Provide a `kubeconfig` file that is a `data` dependency of the test


		The end-to-end tests can be run by executing `e2e-tests.sh --run-tests` in the command line.
		These flags useful for running against an existing cluster, making use of your existing

No test framework #778

No test framework #778

Conversation

bobcatfish commented Apr 29, 2018

Proposed Changes

bobcatfish commented Apr 29, 2018

bobcatfish commented Apr 29, 2018

bobcatfish commented Apr 30, 2018

adrcunha 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

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

adrcunha commented Apr 30, 2018 • edited Loading

bobcatfish commented Apr 30, 2018

adrcunha commented Apr 30, 2018

adrcunha commented Apr 30, 2018

adrcunha commented Apr 30, 2018

google-prow-robot commented Apr 30, 2018

bobcatfish commented Apr 30, 2018

bobcatfish commented Apr 30, 2018 • edited Loading

dprotaso commented May 1, 2018

adrcunha commented May 1, 2018

bobcatfish commented May 1, 2018

bobcatfish commented May 1, 2018

adrcunha commented Apr 30, 2018 •

edited

Loading

bobcatfish commented Apr 30, 2018 •

edited

Loading