Add initial conformance test #193

bobcatfish · 2018-02-19T23:19:58Z

Conformance tests are tests which can be run against any implementation
of the Elafros API to ensure the API has been implemented consistently.
Passing these tests would indicate that apps and functions deployed to
this implementation could be ported to other implementations as well.

The precedent for these tests is the k8s conformance tests which
vendors use to prove they have "certified kubernetes" deployments.
(https://github.com/cncf/k8s-conformance#certified-kubernetes)

This commit includes one conformance test which deploys a new
Route and Configuration from a prebuilt image, then updates the
Configuration to use a different prebuilt image.

Automation to run these tests will be coming later.

The conformance tests use Ginkgo and Gomega:

These are used by the existing k8s conformance tests
Having the same output as the k8s tests will allow us to integrate
with tools that consume the k8s results (e.g. Sonobuoy)
The way tests are structured using Ginkgo (BDD) nicely mirrors the
way the API is specified, e.g. phases of rollout mapping exactly to
steps in a Ginkgo test
Ginkgo output is human readable, widening the range of folks who can
consume and interpret the results

There are two commits which you can look at separately, one which adds tests/docs, the other which adds deps/BUILD (result of ./hack/update-deps.sh).

Fixes #89.

grantr

I'm so happy to have this! Requesting changes because the uploadtestimages.sh script doesn't work from the repo root. Everything else is minor nits.

grantr · 2018-02-20T19:09:23Z

test/conformance/README.md

Grammar nit: seems like there's a missing use here. I'd also add "kubeconfig file" for clarity and consistency with earlier docs.

The tests will use a .kubeconfig file to determine ...

seems like there's a missing use here

hah!

I'd also add "kubeconfig file" for clarity and consistency with earlier docs.

Added!

grantr · 2018-02-20T19:15:10Z

test/conformance/README.md

Grammar nit: article/plural disagreement. I have two suggestions, neither of which is perfect:

A set of built and pushed conformance test images

☝️ could imply there's a specific set of things

All built and pushed conformance test images

☝️ How do I know I have them all?

Changed to:

A docker repo containing the built and pushed conformance test images.

oh oh oh now that we don't need to mention the namespace anymore, i think it can be even simpler:

Your docker repo at DOCKER_REPO_OVERRIDE should contain the conformance test images.

grantr · 2018-02-20T19:20:46Z

test/conformance/README.md

Would be nice to have an example value for $MY_TEST_SUBSET here. Could add it above:

export MY_TEST_SUBSET=function_scaling ginkgo generate $MY_TEST_SUBSET

Also, Kubernetes doc guidelines specify that code snippets shouldn't include command prompts.

I like it!

Thanks for the link to the k8s doc guidelines, I'll try to remove all the command prompts. (I have one confusion with this: sometimes examples are running in certain dirs, it's nice to have some kind of indication of where they expect to be running. Not sure how to handle that without displaying the command prompt, any ides?)

I guess my answer to that would be one of:

Don't make the user change directories (every command is documented as running from the repo root)

Add a preceding step telling the user to change directories

Don't make the user change directories (every command is documented as running from the repo root)

kk, will do, thanks!

grantr · 2018-02-20T19:21:15Z

test/conformance/README.md

Could alternately put the example spec group name here, to correlate it with the CUJ name.

even better!

grantr · 2018-02-20T19:22:35Z

test/conformance/conformance_suite_test.go

Thanks for linking to the explanatory issue! I agree this is mysterious.

grantr · 2018-02-20T19:54:22Z

test/README.md

Typo nit: "conformance"

grantr · 2018-02-20T19:57:26Z

test/conformance/README.md

IMO, this Environment requirements section and the Conformance test images section below should be moved above the Running conformance tests section. Going through the instructions for the first time, I found it slightly jarring to jump to the middle, then the end, then back to the beginning.

kk will move, makes sense!

grantr · 2018-02-20T20:03:51Z

test/conformance/README.md

What do you think about using ~/.kube/config as the default? The ./kubeconfig file is checked in and empty, so not only will the above command never work for first-time users, it will also need to be scrubbed (or omitted) before every commit to avoid accidentally checking in local modifications.

From my dev perspective, the ideal setup would be for the conformance tests to detect the K8S_CLUSTER_OVERRIDE and DOCKER_REPO_OVERRIDE variables so the most common case could be supported without extra testargs.

What do you think about using ~/.kube/config as the default?

I had this is initially, but then I got confused when I tried to make it so that bazel could run the tests. Afaik there is no way for me to make bazel able to use ~/.kube/config, this horrid empty file solution is the best thing I could come up with :(

But! I will make ~/.kube/config the default again, and update the bazel case in the docs to pass a path to the horrible empty file.

(Also in the future i'll be adding automation to run these tests, if that automation uses bazel then we might end up with some content in kubeconfig - I'll add a note about this in the commit message)

grantr · 2018-02-20T20:11:36Z

test/conformance/README.md

I wonder if we could make this verbose output the default? The tests take a while (70 seconds on my machine) so it's nice to see what's happening (and the Ginkgo output is very nice).

I poked around and didn't find a great way to do this. If I come across something I'll come back to it, but I'm also hesitant because:

It would mean -v would always be on for go test which would be kind of odd i flater on we can run all the tests together

It feels weird to take the option away from the user

One way to handle it would be to add a script for invoking the tests which sets these flags - when I come back to this to add automation I think it'll be more clear what's worth scripting and what isn't.

Let me know if you feel strongly though and I'll continue the search!

I'll bump this up so it's the next example command after the bare go test ./test/conformance

👍 I don't feel strongly and I agree with all your points! 😃

grantr · 2018-02-20T20:15:06Z

test/conformance/route_test.go

Typo nit: "retrieve" and "manualy"

adrcunha · 2018-02-20T20:38:54Z

test/README.md

I believe that using just "tests" (instead of "end to end" tests) would be better.

hm the only thing is that the unit tests aren't in this directory 🤔 but I think you're right that just "tests" is better

adrcunha · 2018-02-20T20:40:37Z

test/README.md

Currently the controller/testing tests is failing to build, and cloudbuild/v1alpha1 is not covered.

hm that's not good, not sure what's going on there, gonna have to look into it

okay so it looks like the example test in hooks_test.go is actually broken. which means two things:

it's broken (i'll open a bug about it)

the go_test bazel rule doesn't run testable examples (i'll open a bug about this too but i need a good repro case first)

but either way go test -v ./pkg/... should be a totally legit way to run the tests

🤦‍♂️ I'm embarrassed about the example failure! I've got a fix PR coming shortly.

I'm amazed that Bazel doesn't run example tests. Seems like it would have to go out of its way not to.

adrcunha · 2018-02-20T20:45:25Z

test/conformance/README.md

Can we create a script to make this a kinda no-op process?

So there could be a script that:

makes sure the kubeconfig file exists, has content, and is at the path that bazel expects

makes sure the images have been built and pushed to the docker repo, builds and pushes them if they aren't there

I don't want to add this right now b/c tbh I feel like running these tests with bazel just causes more pain - it's way easier to use go test.

I'll be coming back to this to add automation - and some of that will be cluster setup so I think then it'll be clearer what's worth scripting.

But in the meantime @grantr suggested making ~/.kube/config the default config file, so that will work for most ppl (with go test anyway, not with bazel) so that helps a bit

adrcunha · 2018-02-20T20:52:49Z

test/conformance/README.md

Nit: most filenames in the repo use "_" or "-" to separate words, can you do that also with uploadtestimages.sh?

looks like the scripts are mostly using - so I'll use that

adrcunha · 2018-02-20T20:53:16Z

test/conformance/conformance_suite_test.go

Typo: "required".

adrcunha · 2018-02-20T21:00:06Z

test/conformance/uploadtestimages.sh

Don't we need a copyright header here?

oh yeah! i guess we need that in all the files actually

@adrcunha @grantr do you think I need to add the copyright header to the example image files? it feels like overkill but i'll do it if it's needed

adrcunha · 2018-02-20T21:04:35Z

test/unit_tests.sh

This shouldn't be needed. What motivated you to add this?

i was trying to make it so that the script could be invoked from any directory, however it looks like bazel is already magically and apparently can find //test from any directory anyway so I guess it isn't needed!

vaikas

Thanks so much for doing this!! I'm approving changes from my part assuming the other changes requested get done. I have tiny nits and would like to see this in asap :)

vaikas · 2018-02-21T00:01:48Z

test/conformance/uploadtestimages.sh

tiny nit, in other files you do:
set -ex
which I believe is the same as this?

vaikas · 2018-02-21T00:02:28Z

test/conformance/route_test.go

nit, I think it might be clearer to have isRevisionReady but don't care much.

vaikas · 2018-02-21T00:04:22Z

test/conformance/crdpolling.go

+1 if we're talking about the function name.
I too might prefer something like:
PollUntilRouteReady
or
WaitUntilRouteReady / WaitForRouteReady / WaitRouteRead or something like that, I typically (not necessarily true) think that ExpectXXX means check for XXX and don't poll.

But if others are fine with Poll... I'm fine with that.

bobcatfish · 2018-02-21T18:54:05Z

Okay I think I've addressed all of the feedback (thanks for the detailed review all!!). PTAL.

Conformance tests are tests which can be run against any implementation of the Elafros API to ensure the API has been implemented consistently. Passing these tests would indicate that apps and functions deployed to this implementation could be ported to other implementations as well. The precedent for these tests is the k8s conformance tests which vendors use to prove they have "certified kubernetes" deployments. (https://github.com/cncf/k8s-conformance#certified-kubernetes) This commit includes one conformance test which deploys a new Route and Configuration from a prebuilt image, then updates the Configuration to use a different prebuilt image. Automation to run these tests will be coming later. The current solution to allowing bazel to run these tests is to configure bazel to use an empty kubeconfig file. This is pretty hacky and needs to be revisited later - if we use bazel to run our automated tests, we might put some meaningful values in this file. Fixes #89.

The conformance tests use Ginkgo and Gomega: * These are used by the existing k8s conformance tests * Having the same output as the k8s tests will allow us to integrate with tools that consume the k8s results (e.g. Sonobuoy) * The way tests are structured using Ginkgo (BDD) nicely mirros the way the API is specified, e.g. phases of rollout mapping exactly to steps in a Ginkgo test * Ginkgo output is human readable, widening the range of folks who can consume and interpret the results

Conformance tests are tests which can be run against any implementation of the Elafros API to ensure the API has been implemented consistently. Passing these tests would indicate that apps and functions deployed to this implementation could be ported to other implementations as well. The precedent for these tests is the k8s conformance tests which vendors use to prove they have "certified kubernetes" deployments. (https://github.com/cncf/k8s-conformance#certified-kubernetes) This commit includes one conformance test which deploys a new Route and Configuration from a prebuilt image, then updates the Configuration to use a different prebuilt image. Automation to run these tests will be coming later. The current solution to allowing bazel to run these tests is to configure bazel to use an empty kubeconfig file. This is pretty hacky and needs to be revisited later - if we use bazel to run our automated tests, we might put some meaningful values in this file. The conformance tests use Ginkgo and Gomega: * These are used by the existing k8s conformance tests * Having the same output as the k8s tests will allow us to integrate with tools that consume the k8s results (e.g. Sonobuoy) * The way tests are structured using Ginkgo (BDD) nicely mirros the way the API is specified, e.g. phases of rollout mapping exactly to steps in a Ginkgo test * Ginkgo output is human readable, widening the range of folks who can consume and interpret the results Fixes #89.

Replace canonical_go_repository github.com/knative/serving with knative.dev/serving

* [SRVKS-988] Drop duplicate manifests (knative#81) * Drop duplicate manifests * keep * Drop openshift/release/artifacts * Add correct version * Revert Makefile * Set label version based on the branch name * [SRVKS-1000] Use SO repo's script to install serverless (knative#89) * Bump artifacts manifests

bobcatfish requested review from adrcunha, grantr and vaikas February 19, 2018 23:20

grantr suggested changes Feb 20, 2018

View reviewed changes

adrcunha suggested changes Feb 20, 2018

View reviewed changes

vaikas approved these changes Feb 21, 2018

View reviewed changes

bobcatfish force-pushed the conformance branch 3 times, most recently from 874e96b to 9b03f38 Compare February 21, 2018 18:41

grantr approved these changes Feb 21, 2018

View reviewed changes

bobcatfish force-pushed the conformance branch from 9b03f38 to 806f4f5 Compare February 21, 2018 18:52

bobcatfish added 2 commits February 21, 2018 10:58

bobcatfish force-pushed the conformance branch from 806f4f5 to c5d07a0 Compare February 21, 2018 18:58

grantr mentioned this pull request Feb 21, 2018

Use the correct signature for example test #202

Merged

adrcunha approved these changes Feb 21, 2018

View reviewed changes

bobcatfish merged commit 351f6b0 into knative:master Feb 21, 2018

bobcatfish deleted the conformance branch February 21, 2018 22:50

markusthoemmes pushed a commit to markusthoemmes/knative-serving that referenced this pull request Jul 15, 2019

Merge pull request knative#193 from nak3/knative.dev

2d2870b

Replace canonical_go_repository github.com/knative/serving with knative.dev/serving

Add initial conformance test #193

Add initial conformance test #193

Uh oh!

Conversation

bobcatfish commented Feb 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grantr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrcunha Feb 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrcunha Feb 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobcatfish commented Feb 19, 2018 •

edited

Loading

adrcunha Feb 20, 2018 •

edited

Loading

adrcunha Feb 20, 2018 •

edited

Loading

adrcunha Feb 20, 2018 •

edited

Loading