Add E2E tests for trigger #645

daisy-ycguo · 2020-02-10T07:56:34Z

Fixes #568

Proposed Changes

Add e2e tests for trigger
Add retry mechanism in trigger update when meeting with conflicting error.

rhuss

Looks good, but aren't you missing some tests for which you already prepared some private methods ? (like update, delete, or with error) ?

rhuss · 2020-02-10T10:19:27Z

test/e2e/trigger_test.go

+	assert.Check(t, util.ContainsAllIgnoreCase(out, "Trigger", name, "created", "namespace", test.kn.namespace))
+}
+
+func (test *e2eTest) triggerCreateMissingSink(t *testing.T, name string, resources string, sa string, sink string) {


where is this test called ?

Thank you for the review @rhuss. Yes, this is not completed. still working on other test cases

navidshaikh · 2020-02-10T09:20:11Z

test/e2e/trigger_test.go

+
+	_, err := kubectl.RunWithOpts([]string{"label", "namespace", test.kn.namespace, "knative-eventing-injection=enabled"}, runOpts{})
+	if err != nil {
+		t.Fatalf("Error executing 'kubectl lable namespace %s knative-eventing-injection=enabled'. Error: %s", test.kn.namespace, err.Error())


Suggested change

t.Fatalf("Error executing 'kubectl lable namespace %s knative-eventing-injection=enabled'. Error: %s", test.kn.namespace, err.Error())

t.Fatalf("Error executing 'kubectl label namespace %s knative-eventing-injection=enabled'. Error: %s", test.kn.namespace, err.Error())

Thanks, will fix it in next commit.

rhuss · 2020-02-10T15:04:28Z

/retest

rhuss · 2020-02-10T15:43:21Z

@daisy-ycguo there seems to be compile errors:

# knative.dev/client/test/e2e [knative.dev/client/test/e2e.test]
test/e2e/trigger_test.go:54:28: undefined: Interval
test/e2e/trigger_test.go:54:38: undefined: Timeout

maximilien

Incomplete...

maximilien · 2020-02-10T21:22:30Z

test/e2e/trigger_test.go

+	}
+
+	return wait.PollImmediate(Interval, Timeout, func() (bool, error) {
+		out, err := kubectl.RunWithOpts([]string{"get", "broker", "-n", test.kn.namespace, "-o=jsonpath='{.items[0].status.conditions[?(@.type==\"Ready\")].status}'"}, runOpts{})


Please check this err:
assert.NilError(t, err)
no point in propagating as this is a test

exactly. checked in the new version.

daisy-ycguo · 2020-02-11T02:53:04Z

/test pull-knative-client-integration-tests

rhuss · 2020-02-11T09:07:23Z

=== RUN   TestBasicWorkflow/create_hello_service_and_return_no_error
Running 'kn service create hello --image gcr.io/knative-samples/helloworld-go -n kne2etests6'...
Failed to successfully execute 'kubectl lable namespace kne2etests7 knative-eventing-injection=enabled': Execution error: stderr: 'Error: unknown command "lable" for "kubectl"

Did you mean this?
	label

Run 'kubectl --help' for usage.
unknown command "lable" for "kubectl"

Did you mean this?
	label

' error: 'exit status 1'FAIL	knative.dev/client/test/e2e	1.969s
FAIL

rhuss · 2020-02-11T10:57:55Z

test/e2e/trigger_test.go

+func (test *e2eTest) triggerCreate(t *testing.T, name string, filters []string, sinksvc string) {
+	filterStr := ""
+	for _, v := range filters {
+		filterStr = filterStr + "--filter " + v + " "


You can't concatenate the filters like this in a string as this will then considered to be a single argument (not multiple ones as you would expect). See also the error in the integration test.

Instead you should put all in a string array, adding the broked and sink args in the end and then use that slice to put into the RunWithOpts call (with "..." to convert to varargs)

yeah. fixing it.

knative-metrics-robot · 2020-02-12T00:13:51Z

The following is the coverage report on the affected files.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/kn/commands/trigger/update.go	86.7%	80.4%	-6.3

daisy-ycguo · 2020-02-12T01:40:44Z

/test pull-knative-client-integration-tests

daisy-ycguo · 2020-02-12T03:09:46Z

@rhuss @maximilien @navidshaikh This PR is ready for review and merge if possible. Thanks.

rhuss

thanks, lgtm !

/lgtm

knative-prow-robot · 2020-02-12T11:32:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: daisy-ycguo, maximilien, rhuss

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~~ [maximilien,rhuss]

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

navidshaikh · 2020-02-12T14:00:09Z

/retest

Failed to successfully execute 'kn service create hello --image gcr.io/knative-samples/helloworld-go -n kne2etests1': Execution error: stderr: 'timeout: service 'hello' not ready after 600 seconds

and

    --- FAIL: TestTrafficSplit/tag_latest_ready_revision_of_service_as_current (4.62s)

ref: logs

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Feb 10, 2020

knative-prow-robot requested review from cppforlife and rhuss February 10, 2020 07:56

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 10, 2020

daisy-ycguo force-pushed the triggere2e branch 3 times, most recently from fc76456 to 54d1e78 Compare February 10, 2020 08:46

rhuss reviewed Feb 10, 2020

View reviewed changes

navidshaikh reviewed Feb 10, 2020

View reviewed changes

daisy-ycguo changed the title ~~Add E2E tests for trigger~~ WIP: Add E2E tests for trigger Feb 10, 2020

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 10, 2020

maximilien approved these changes Feb 10, 2020

View reviewed changes

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 10, 2020

daisy-ycguo force-pushed the triggere2e branch 4 times, most recently from abd234e to 16f1715 Compare February 11, 2020 08:28

daisy-ycguo changed the title ~~WIP: Add E2E tests for trigger~~ Add E2E tests for trigger Feb 11, 2020

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 11, 2020

daisy-ycguo force-pushed the triggere2e branch 2 times, most recently from e113654 to 5dca113 Compare February 11, 2020 08:44

daisy-ycguo force-pushed the triggere2e branch 2 times, most recently from 1261c06 to 27996da Compare February 11, 2020 09:44

rhuss reviewed Feb 11, 2020

View reviewed changes

daisy-ycguo force-pushed the triggere2e branch 2 times, most recently from 96ff88c to 21e56cd Compare February 11, 2020 13:20

daisy-ycguo force-pushed the triggere2e branch 5 times, most recently from 8571a03 to f928f6c Compare February 11, 2020 16:14

Daisy Guo added 2 commits February 12, 2020 00:14

add integration test for trigger

f928f6c

update doc

fe970ac

rhuss approved these changes Feb 12, 2020

View reviewed changes

knative-prow-robot assigned rhuss Feb 12, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 12, 2020

knative-prow-robot merged commit e031917 into knative:master Feb 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add E2E tests for trigger #645

Add E2E tests for trigger #645

daisy-ycguo commented Feb 10, 2020 •

edited

Loading

rhuss left a comment

rhuss Feb 10, 2020

daisy-ycguo Feb 10, 2020

navidshaikh Feb 10, 2020

daisy-ycguo Feb 10, 2020

rhuss commented Feb 10, 2020

rhuss commented Feb 10, 2020

maximilien left a comment

maximilien Feb 10, 2020

daisy-ycguo Feb 11, 2020

daisy-ycguo commented Feb 11, 2020

rhuss commented Feb 11, 2020

rhuss Feb 11, 2020

daisy-ycguo Feb 11, 2020

knative-metrics-robot commented Feb 12, 2020

daisy-ycguo commented Feb 12, 2020

daisy-ycguo commented Feb 12, 2020

rhuss left a comment

knative-prow-robot commented Feb 12, 2020

navidshaikh commented Feb 12, 2020

	t.Fatalf("Error executing 'kubectl lable namespace %s knative-eventing-injection=enabled'. Error: %s", test.kn.namespace, err.Error())
	t.Fatalf("Error executing 'kubectl label namespace %s knative-eventing-injection=enabled'. Error: %s", test.kn.namespace, err.Error())

Add E2E tests for trigger #645

Add E2E tests for trigger #645

Conversation

daisy-ycguo commented Feb 10, 2020 • edited Loading

Proposed Changes

rhuss 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

rhuss commented Feb 10, 2020

rhuss commented Feb 10, 2020

maximilien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daisy-ycguo commented Feb 11, 2020

rhuss commented Feb 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knative-metrics-robot commented Feb 12, 2020

daisy-ycguo commented Feb 12, 2020

daisy-ycguo commented Feb 12, 2020

rhuss left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Feb 12, 2020

navidshaikh commented Feb 12, 2020

daisy-ycguo commented Feb 10, 2020 •

edited

Loading