EventListener e2e test #53

ncskier · 2019-07-30T23:09:47Z

Changes

Helper functions for initializing & tearing down the e2e tests
EventListener e2e test
README for tests

WIP

~~Need to add README for tests~~
~~Questions~~
- ~~We don't need the TestMain function that Pipelines has do we?~~
  We do not need the TestMain function
No builder package at the moment, so I'm creating the EventListener manually

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)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

bobcatfish

We don't need the TestMain function that Pipelines has do we?

eek, interestingly I don't think Pipelines itself needs this function anymore either! when it was added in tektoncd/pipeline@fbad053 it used to do a lot more and looks like it's probably totally unnecessary now 😅

test/clients.go

test/eventlistener_test.go

bobcatfish · 2019-07-31T03:10:43Z

test/init_test.go

+		if knativetest.Flags.EmitMetrics {
+			logging.InitializeMetricExporter(t.Name())
+		}
+	})


if there is any way not to include this it would probably be good, it's not clear to me that we can even use the metrics exporting (i might be wrong tho!)

The plumbing repo currently supports the --emit-metrics flag (see here). However, I don't know if anyone's using this option. @vdemeester do you know if the --emit-metrics option is being used?

On tektoncd it is not being used I don't think

Ok, thanks Vincent. I don't know if I should keep it then. Since the plumbing repo currently references an --emit-metrics flag, I'm leaning toward keeping it in. However, I don't feel strongly either way 😁

Metrics will be emitted for these Wait methods tracking how long test poll for.

(from pipeline's test/README)
Hm, looks like the pipeline's wait library is emitting metrics. This data could be useful in the future, although I don't know if anyone's using this data right now.

bobcatfish · 2019-07-31T03:13:24Z

test/init_test.go

+
+func createNamespace(t *testing.T, namespace string, kubeClient kubernetes.Interface) {
+	t.Helper()
+	t.Logf("Create namespace %s to deploy to", namespace)


maybe this is something you can tell me from running the tests yourself: im under the impression that the t.Log functions buffer their output and therefore won't show up until a test is done (or some random flush point during the test). If that's the case, then i think using log is better b/c folks running these tests can see incremental progress (since they can be slow)

(This same issue exists in pipelines and is on my sad secret TODO list to fix :(((( - if you can confirm this behaviour i pledge to make a real issue in the pipeline backlog about it asap XD)

So, from what I understand, not using t.Logf can mess up where your logs get printed. So while they might be logged in "real time," the logs might look jumbled (some info here https://stackoverflow.com/questions/23205419/how-do-you-print-in-a-go-test-using-the-testing-package).

There is an approved proposal open to stream t.Log output. Looks interesting, although I don't know if/when it will be available: golang/go#24929.

bobcatfish · 2019-07-31T03:14:03Z

test/init_test.go

+		return true, err
+	}); err != nil {
+		t.Fatalf("Failed to get SA %q in namespace %q for tests: %s", defaultSA, namespace, err)
+	}


hm, can you explain a bit more about the purpose of this check? i would think it would be safe to assume the default account always exists?

I put it in because that's what the Pipelines project does 😅 But I can definitely remove this check if it isn't necessary.

Maybe pipelines needs this in case no service account is provided so that there is a fall back?

ncskier · 2019-07-31T12:41:07Z

In the pull-tekton-triggers-integration-tests, I am getting this error

Failed to create kubernetes clientset from config file at /root/.kube/config: No Auth Provider found for name "gcp"

The error comes from trying to initialize the kubernetes clientset here.
@bobcatfish @vdemeester how is the test cluster set up? Do we need to do some additional setup to the test cluster to fix this error?

UPDATE
Oh wait, this should probably fix it 😅 🤞
|-> Yep, fixed it

vtereso

Minor points in the chat (which logger, whether checks are unnecessary) shouldn't hold this back. We have a lot in flux right now so closing this seems worthwhile.

tekton-robot · 2019-08-06T18:34:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ncskier, vtereso

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~~ [ncskier,vtereso]

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

vtereso · 2019-08-06T18:37:14Z

/lgtm

tekton-robot requested review from iancoffey and vtereso July 30, 2019 23:09

tekton-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 30, 2019

bobcatfish reviewed Jul 31, 2019

View reviewed changes

ncskier force-pushed the issue37 branch from 0bab149 to 1e020b2 Compare July 31, 2019 12:17

ncskier force-pushed the issue37 branch from e612f2f to 3ae4552 Compare July 31, 2019 12:56

tekton-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 31, 2019

ncskier force-pushed the issue37 branch 2 times, most recently from 65ecb57 to cc344fd Compare July 31, 2019 14:59

ncskier mentioned this pull request Jul 31, 2019

Define EventListener Status #54

Closed

ncskier force-pushed the issue37 branch 4 times, most recently from 002b091 to 70a8b69 Compare August 1, 2019 16:48

ncskier marked this pull request as ready for review August 1, 2019 18:09

ncskier requested a review from bobcatfish August 1, 2019 18:14

Add e2e test that creates/deletes an EventListener

5630510

ncskier force-pushed the issue37 branch from 70a8b69 to 5630510 Compare August 6, 2019 16:05

vtereso approved these changes Aug 6, 2019

View reviewed changes

tekton-robot assigned vtereso Aug 6, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 6, 2019

tekton-robot merged commit 79d449a into tektoncd:master Aug 6, 2019

ncskier mentioned this pull request Aug 7, 2019

Issue38 test builders #52

Merged

3 tasks

ncskier deleted the issue37 branch August 7, 2019 22:23

ncskier mentioned this pull request Aug 7, 2019

Create an end to end test (in go) that creates an EventListener #37

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EventListener e2e test #53

EventListener e2e test #53

ncskier commented Jul 30, 2019 •

edited

Loading

bobcatfish left a comment

bobcatfish Jul 31, 2019

ncskier Jul 31, 2019

vdemeester Jul 31, 2019

ncskier Jul 31, 2019

ncskier Aug 1, 2019

vdemeester Aug 1, 2019

bobcatfish Jul 31, 2019

ncskier Aug 1, 2019

bobcatfish Jul 31, 2019

ncskier Jul 31, 2019

vtereso Aug 6, 2019

ncskier commented Jul 31, 2019 •

edited

Loading

vtereso left a comment

tekton-robot commented Aug 6, 2019

vtereso commented Aug 6, 2019

EventListener e2e test #53

EventListener e2e test #53

Conversation

ncskier commented Jul 30, 2019 • edited Loading

Changes

WIP

Submitter Checklist

bobcatfish 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

ncskier commented Jul 31, 2019 • edited Loading

vtereso left a comment

Choose a reason for hiding this comment

tekton-robot commented Aug 6, 2019

vtereso commented Aug 6, 2019

ncskier commented Jul 30, 2019 •

edited

Loading

ncskier commented Jul 31, 2019 •

edited

Loading