Execute yaml examples via go tests #2541

bobcatfish · 2020-05-05T00:36:44Z

Changes

In #2540 we are seeing that some yaml tests are timing out, but it's
hard to see what yaml tests are failing. This commit moves the logic out
of bash and into individual go tests - now we will run an individual go
test for each yaml example, completing all v1alpha1 before all v1beta1
and cleaning up in between. The output will still be challenging to read
since it will be interleaved, however the failures should at least
be associated with a specific yaml file.

This also makes it easier to run all tests locally, though if you
interrupt the tests you end up with your cluster in a bad state and it
might be good to update these to execute each example in a separate
namespace (in which case we could run all of v1alpha1 and v1beta1 at the
same time as well!)

I have a feeling this won't work on the first try and that I've still go a few issues to work out, not to mention that the code is a bit icky, esp. since im using t.Helper so profusely.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
[n/a] Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

imjasonh

This is definitely progress! 🙌

imjasonh · 2020-05-05T01:15:08Z

examples/yaml_test.go

+	var stderr, stdout bytes.Buffer
+	cmd.Stderr = &stderr
+	cmd.Stdout = &stdout
+	if err := cmd.Run(); err != nil {


Maybe we can use CombinedOutput instead?

TIL! thanks :D

I'm not sure I do want to use combinedoutput in this case tho, cuz im using stderr if the command fails, and if not, im returning stdout. in the error case, dumping both seems fine, but in the success case, not including stderr seems like it makes sense to me. what do you think?

imjasonh · 2020-05-05T01:16:36Z

examples/yaml_test.go

+		"serviceaccounts",
+		"persistentvolumeclaims",
+	}
+	for _, c := range crdTypes {


Eventually we should be able to parse the YAML file and create the resource using a client, instead of involving kubectl.

For this specifically we can just use the CRD client to delete the types.

It'd be fine to have a TODO for that for now but that's a better final state I think.

Yes and no. We will need to also test some kubectl create commands as this is something the user will do.

I'm not 100% sure which is better - if we want to use the clients, we need to load the yamls + find find the right client for the thing being created. i think im like 60% convinced this is better in the long run, but there's also something nice about just blasting it all out and watching it. i do think the part where i look for the word "run" in the ko output is pretty hacky, ill put a comment in about that for sure

imjasonh · 2020-05-05T01:18:21Z

examples/yaml_test.go

+
+// replaceDockerRepo will look in the content f and replace the hard coded docker
+// repo with the on provided via the KO_DOCKER_REPO environment variable
+func replaceDockerRepo(t *testing.T, f string) string {


Ugh, this is kinda gross. I think we could just drop any test that requires ko-building a package?

we could - this is what the original yaml tests do tho, do you mind if i keep this in the context of this pull request and remove in another PR?

imjasonh · 2020-05-05T01:18:57Z

examples/yaml_test.go

+
+// pollRun will use kubectl to query the specified run to see if it
+// has completed. It will timeout after timeoutSeconds.
+func pollRun(t *testing.T, run string, wg *sync.WaitGroup) {


This could Watch instead and maybe end up faster?

hm could you give me an example of how that would work? ive never used kubectl watch - trying it out now it seems like id have to stream output from it which seems a bit more complicated given im calling kubectl with exec.Command - i think im being a bit lazy for sure but this seems pretty much fine for now? i can put in a comment to explore using watch

imjasonh · 2020-05-05T01:19:52Z

test/e2e-tests-yaml.sh

-  done
-done
-
+failed=$(go test -timeout 15m ./examples)


🙌🙌🙌🙌🙌

vdemeester · 2020-05-05T08:36:35Z

level=error msg="Running error: context loading failed: failed to load program with go/packages: could not parse GOARCH and Go compiler in format \"<GOARCH> <compiler>\" from stdout of go command:\nGOROOT=/usr/local/go GOPATH=/home/prow/go GO111MODULE= PWD=/home/prow/go/src/github.com/tektoncd/pipeline go [list -f {{context.GOARCH}} {{context.Compiler}} -tags e2e -mod=vendor -- unsafe]\ndir: /home/prow/go/src/github.com/tektoncd/pipeline\nstdout: <<>>\nstderr: <<go: inconsistent vendoring in /home/prow/go/src/github.com/tektoncd/pipeline:\n\tgithub.com/google/[email protected]: is explicitly required in go.mod, but vendor/modules.txt indicates github.com/google/[email protected]\n\tgithub.com/pkg/[email protected]: is explicitly required in go.mod, but vendor/modules.txt indicates github.com/pkg/[email protected]\n\nrun 'go mod vendor' to sync, or use -mod=mod or -mod=readonly to ignore the vendor directory\n>>"

The build failure is interesting 😛

bobcatfish · 2020-05-21T21:15:32Z

/test check-pr-has-kind-label

bobcatfish · 2020-05-21T22:05:54Z

I dunno what was up with that weird prow error but it stopped :D

This should be ready for a real review now! Might still be some kinks to work out...

I'm hoping after this we could migrate these tests to a separate test triggered via prow maybe, tho it's a bit more complicated b/c we'll have to invoke boskos too...

ghost · 2020-05-26T13:40:34Z

Looks like this would fix #1251 ?

Also looks like there's a similarly-intentioned PR here: #2685 ?

bobcatfish · 2020-05-26T14:13:57Z

examples/admission/tasks.yaml

+                #!/usr/bin/env bash
+
+                # TODO: assert something about the expected contents of $(params.output)
+                # TODO: assert something about the expected results in the cluster


bobcatfish · 2020-05-26T14:26:23Z

Looks like this would fix #1251 ?

🤦‍♀️ my bad for not seeing that sooner! If this next round of tests passes, im suggesting (#2685 (comment) ) we merge this and then add improvements from #2685 on top - means more work for @thomaschandler tho :(

tekton-robot · 2020-05-26T15:44:31Z

This PR cannot be merged: expecting exactly one kind/ label

Available kind/ labels are:

kind/bug: Categorizes issue or PR as related to a bug.
kind/flake: Categorizes issue or PR as related to a flakey test
kind/cleanup: Categorizes issue or PR as related to cleaning up code, process, or technical debt.
kind/design: Categorizes issue or PR as related to design.
kind/documentation: Categorizes issue or PR as related to documentation.
kind/feature: Categorizes issue or PR as related to a new feature.
kind/misc: Categorizes issue or PR as a miscellaneuous one.

ghost · 2020-05-26T16:12:49Z

test/e2e-tests.sh

@@ -30,8 +30,9 @@ install_pipeline_crd
 failed=0

 # Run the integration tests
-header "Running Go e2e tests"
-go_test_e2e -timeout=20m ./test/... || failed=1
+# TODO HACK HACK HACK HACK


Is this dead code? If not could the comment do a bit more to describe why these lines have been left in but commented out?

This is the bit that runs our integration tests! Unfortunately they all run BEFORE the yaml tests so I had temporarily commented them out so I could check if the tests were succeeding without having to wait for these to complete 🤦‍♀️

In the long run hopefully we can run them in parallel! The main complication that stops us from doing that right away is that both tests require a boskos cluster

ghost · 2020-05-26T16:17:06Z

examples/examples_test.go

+	if err != nil {
+		t.Fatalf("couldnt read contents of %s: %v", f, err)
+	}
+	return strings.Replace(string(read), "gcr.io/christiewilson-catfactory", r, -1)


Would it be worth putting "gcr.io/christiewilson-catfactory" into a named constant? I'm a bit confused why it appears here.

In tektoncd#2540 we are seeing that some yaml tests are timing out, but it's hard to see what yaml tests are failing. This commit moves the logic out of bash and into individual go tests - now we will run an individual go test for each yaml example, completing all v1alpha1 before all v1beta1 and cleaning up in between. The output will still be challenging to read since it will be interleaved, however the failures should at least be associated with a specific yaml file. This also makes it easier to run all tests locally, though if you interrupt the tests you end up with your cluster in a bad state and it might be good to update these to execute each example in a separate namespace (in which case we could run all of v1alpha1 and v1beta1 at the same time as well!)

There's some good stuff in this doc but it's hard to remember what's in it cuz it's kinda all over the place - maybe a TOC will help!

tekton-robot · 2020-05-26T17:59:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg

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~~ [sbwsg]

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

bobcatfish · 2020-05-27T20:23:21Z

can has lgtm?

afrittoli

Thank you for this, looking great!
The only concern I have is about stdout in case of error, but that's something we can improve on later.

afrittoli · 2020-05-27T21:14:14Z

examples/examples_test.go

+	cmd.Stderr = &stderr
+	cmd.Stdout = &stdout
+	if err := cmd.Run(); err != nil {
+		logf("couldn't run command %s %v: %v, %s", c, args, err, stderr.String())


It looks to me like in case of error we do not get to see the stdout at all.
I think we should print out both out and err, perhaps in two different log commands.

I tend to agree, if the command failed, I tend to want to see stderr and stdout.
gotest.tools/v3/icmd would come handy there 😝

afrittoli · 2020-05-27T21:19:09Z

examples/examples_test.go

+			return
+		}
+
+		switch status {


In future we might want to allow for example metadata of some kind (annotations?) to specify an expected target status and more details about it.

afrittoli · 2020-05-27T21:24:58Z

examples/examples_test.go

+}
+
+// getYamls will look in the directory in examples indicated by version and run for yaml files
+func getYamls(t *testing.T, version, run string) []string {


I wonder if some of this helpers could use a unit test... we do run them as part of the tests anyways, so they most likely all do that they are expected to :)

haha probably!!!

afrittoli · 2020-05-27T21:28:21Z

/hold

afrittoli · 2020-05-27T21:30:17Z

I'm afraid something is wrong, the YAML tests are timing out, logged as "FAILED" but still the CI job is marked as green:

panic: test timed out after 15m0s

(...)

goroutine 2833 [IO wait]:
internal/poll.runtime_pollWait(0x7f23b8a5d008, 0x72, 0xffffffffffffffff)
	/usr/local/go/src/runtime/netpoll.go:203 +0x55
internal/poll.(*pollDesc).wait(0xc00038a4f8, 0x72, 0x501, 0x5de, 0xffffffffffffffff)
	/usr/local/go/src/internal/poll/fd_poll_runtime.go:87 +0x45
internal/poll.(*pollDesc).waitRead(...)
	/usr/local/go/src/internal/poll/fd_poll_runtime.go:92
internal/poll.(*FD).Read(0xc00038a4e0, 0xc00027ec22, 0x5de, 0x5de, 0x0, 0x0, 0x0)
	/usr/local/go/src/internal/poll/fd_unix.go:169 +0x19b
os.(*File).read(...)
	/usr/local/go/src/os/file_unix.go:263
os.(*File).Read(0xc0005ea328, 0xc00027ec22, 0x5de, 0x5de, 0x22, 0x0, 0x0)
	/usr/local/go/src/os/file.go:116 +0x71
bytes.(*Buffer).ReadFrom(0xc000589ef0, 0x13c8460, 0xc0005ea328, 0x7f23b9c24660, 0xc000589ef0, 0xc0005f0701)
	/usr/local/go/src/bytes/buffer.go:204 +0xb1
io.copyBuffer(0x13c7120, 0xc000589ef0, 0x13c8460, 0xc0005ea328, 0x0, 0x0, 0x0, 0x406ca5, 0xc0004ae360, 0xc0005f07b0)
	/usr/local/go/src/io/io.go:391 +0x2fc
io.Copy(...)
	/usr/local/go/src/io/io.go:364
os/exec.(*Cmd).writerDescriptor.func1(0xc0004ae360, 0xc0005f07b0)
	/usr/local/go/src/os/exec/exec.go:310 +0x63
os/exec.(*Cmd).Start.func1(0xc0005e7600, 0xc00069bb00)
	/usr/local/go/src/os/exec/exec.go:436 +0x27
created by os/exec.(*Cmd).Start
	/usr/local/go/src/os/exec/exec.go:435 +0x608
FAIL	github.com/tektoncd/pipeline/examples	900.032s
FAIL'
+ ((  failed  ))

bobcatfish · 2020-05-27T22:02:35Z

whoa, that's no good at all! thanks for noticing that @afrittoli 🙏

thomaschandler · 2020-06-01T23:50:49Z

@bobcatfish I've managed to get tests passing on #2685. How would you feel about merging #2685 instead of this MR?

vdemeester

Few comments 👼

vdemeester · 2020-06-02T16:16:46Z

examples/examples_test.go

+	"sync"
+	"testing"
+	"time"
+


nit: extra space not needed 😝

vdemeester · 2020-06-02T16:17:59Z

examples/examples_test.go

+	// we may want to consider either not running examples that require registry access
+	// or doing something more sophisticated to inject the right registry in when folks
+	// are executing the examples
+	horribleHardCodedRegistry = "gcr.io/christiewilson-catfactory"


nit: Why not running a local registry (in the test namespace) ? (in any case, it would be a follow-up)

could be a good follow up! unless the test is deploying to a different namespace 🤔

vdemeester · 2020-06-02T16:19:31Z

examples/examples_test.go

+	cmd.Stderr = &stderr
+	cmd.Stdout = &stdout
+	if err := cmd.Run(); err != nil {
+		logf("couldn't run command %s %v: %v, %s", c, args, err, stderr.String())


I tend to agree, if the command failed, I tend to want to see stderr and stdout.
gotest.tools/v3/icmd would come handy there 😝

vdemeester · 2020-06-02T16:21:12Z

examples/examples_test.go

+	t.Helper()
+	r := os.Getenv("KO_DOCKER_REPO")
+	if r == "" {
+		t.Fatalf("KO_DOCKER_REPO must be set")


Should we error out or skip ? (like we do on the e2e go tests — mainly to not break openshift-pipelines CI 😝 )

vdemeester · 2020-06-02T16:22:53Z

examples/examples_test.go

+
+// logRun will retrieve the entire yaml of run in namespace n and log it
+func logRun(t *testing.T, n, run string) {
+	t.Helper()


t.Helper might no be needed here (as it is called by other func that are already calling t.Helper) — see stretchr/testify#933

vdemeester · 2020-06-02T16:27:53Z

examples/examples_test.go

+	for i := 0; i < (timeoutSeconds / sleepBetween); i++ {
+		status, err := cmd(t.Logf, "kubectl", []string{"--namespace", n, "get", run, "--output=jsonpath={.status.conditions[*].status}"}, "")
+		if err != nil {
+			t.Fatalf("couldnt get status of %s: %v", run, err)


t.Fatalf does t.FailNow which calls runtime.Goexit, so wg.Done() seems unecessary 🙃

This means it will quit the test on this error… If that's not what we want, we need to use t.Errorf.

vdemeester · 2020-06-02T16:30:57Z

examples/examples_test.go

+
+			t.Logf("Applying %s to namespace %s", y, n)
+			content := replaceDockerRepo(t, fmt.Sprintf("%s/%s/%s", version, run, y))
+			output, err := cmd(t.Logf, "ko", []string{"create", "--namespace", n, "-f", "-"}, content)


s/ko/kubectl 🤔 ⁉️

the existing scripts are using ko :O

vdemeester · 2020-06-02T16:33:04Z

examples/examples_test.go

+// getYamls will look in the directory in examples indicated by version and run for yaml files
+func getYamls(t *testing.T, version, run string) []string {
+	t.Helper()
+	_, filename, _, _ := runtime.Caller(0)


Any reason to make the test dependent on the test file ? 🤔 (and use runtime.Caller)

bobcatfish · 2020-06-02T19:56:33Z

@bobcatfish I've managed to get tests passing on #2685. How would you feel about merging #2685 instead of this MR?

@thomaschandler good call, I think you're able to get to this faster than me! lemme go over to #2685 and review and we can merge your PR instead/first :D

bobcatfish · 2020-06-02T20:51:53Z

Apologies for closing this after your careful review @vdemeester @afrittoli @imjasonh but @thomaschandler is making much faster progress over in #2685 and he's using the client libs so it's a bit cleaner so im closing this PR in favor of it.

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 5, 2020

tekton-robot requested review from imjasonh and vdemeester May 5, 2020 00:36

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 5, 2020

bobcatfish force-pushed the yaml_blamo branch from 0f6ea8e to 35fc0b3 Compare May 5, 2020 00:37

imjasonh requested changes May 5, 2020

View reviewed changes

vdemeester mentioned this pull request May 5, 2020

yaml tests seem to be consistently timing out #2540

Closed

ghost added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label May 21, 2020

bobcatfish force-pushed the yaml_blamo branch 4 times, most recently from 7aac3b2 to 375d820 Compare May 21, 2020 20:50

bobcatfish force-pushed the yaml_blamo branch from 375d820 to bbac45a Compare May 21, 2020 22:04

bobcatfish marked this pull request as ready for review May 21, 2020 22:05

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 21, 2020

ghost mentioned this pull request May 26, 2020

Run CI YAML Example tests using go test #2685

Merged

3 tasks

bobcatfish commented May 26, 2020

View reviewed changes

bobcatfish force-pushed the yaml_blamo branch from bbac45a to 8511459 Compare May 26, 2020 14:16

bobcatfish added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels May 26, 2020

bobcatfish mentioned this pull request May 26, 2020

check-pr-has-kind-label should keep applying until fails or succeeds tektoncd/plumbing#400

Closed

ghost reviewed May 26, 2020

View reviewed changes

bobcatfish added 2 commits May 26, 2020 13:06

Add table of contents to test README 📝

a9206d9

There's some good stuff in this doc but it's hard to remember what's in it cuz it's kinda all over the place - maybe a TOC will help!

bobcatfish force-pushed the yaml_blamo branch from 8511459 to a9206d9 Compare May 26, 2020 17:07

ghost approved these changes May 26, 2020

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 26, 2020

afrittoli reviewed May 27, 2020

View reviewed changes

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 27, 2020

vdemeester reviewed Jun 2, 2020

View reviewed changes

bobcatfish closed this Jun 2, 2020

Execute yaml examples via go tests #2541

Execute yaml examples via go tests #2541

Conversation

bobcatfish commented May 5, 2020

Changes

Submitter Checklist

Reviewer Notes

imjasonh 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

vdemeester commented May 5, 2020

bobcatfish commented May 21, 2020

bobcatfish commented May 21, 2020

ghost commented May 26, 2020

Choose a reason for hiding this comment

bobcatfish commented May 26, 2020

tekton-robot commented May 26, 2020

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

tekton-robot commented May 26, 2020

bobcatfish commented May 27, 2020

afrittoli 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

afrittoli commented May 27, 2020

afrittoli commented May 27, 2020

bobcatfish commented May 27, 2020

thomaschandler commented Jun 1, 2020 • edited Loading

vdemeester 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

bobcatfish commented Jun 2, 2020

bobcatfish commented Jun 2, 2020

thomaschandler commented Jun 1, 2020 •

edited

Loading