Add event message verification to cronjob e2e test #698

daisy-ycguo · 2020-03-02T09:02:12Z

Fixes #566

Proposed Changes

Add event message verification to cronjob e2e test

knative-prow-robot · 2020-03-02T09:02:42Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: daisy-ycguo
To complete the pull request process, please assign maximilien
You can assign the PR to them by writing /assign @maximilien in a comment when ready.

The full list of commands accepted by this bot can be found 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

maximilien

Left some feedback. Also, tests are failing.

maximilien · 2020-03-02T23:55:22Z

test/e2e/cronjob_test.go

+	assert.NilError(t, err)
+}
+
+func (test *e2eTest) verifyDeventDisplayLogs(t *testing.T, svcname string, message string) error {


s/verifyDeventDisplayLogs/verifyEventDisplayLogs/

maximilien · 2020-03-02T23:55:43Z

test/e2e/cronjob_test.go

+		out     string
+	)
+
+	selectorstr := fmt.Sprintf("serving.knative.dev/service=%s", svcname)


s/selectorstr/selectorStr/

maximilien · 2020-03-02T23:56:27Z

test/e2e/cronjob_test.go

+
+	selectorstr := fmt.Sprintf("serving.knative.dev/service=%s", svcname)
+	for retries < 5 {
+		out, err = kubectl{}.Run("logs", "-l", selectorstr, "-c", "user-container")


I'd separate the retry logic and code into it's on private function.

Would also simplify this function.

maximilien · 2020-03-02T23:57:09Z

test/e2e/cronjob_test.go

+	}
+}
+
+func (test *e2eTest) serviceCreateEventDisplay(t *testing.T, r *KnRunResultCollector, serviceName string) {


Why is this not a public Test... function?

It's same as serviceCreate function defined in service_test.go

func (test *e2eTest) serviceCreateDuplicate(t *testing.T, r *KnRunResultCollector, serviceName string)

Although it's not public, but you can use this function under any e2e tests.

knative-prow-robot · 2020-03-03T08:39:44Z

@daisy-ycguo: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-knative-client-integration-tests	`f564a3c`	link	`/test pull-knative-client-integration-tests`
pull-knative-client-integration-tests-latest-release	`f564a3c`	link	`/test pull-knative-client-integration-tests-latest-release`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

daisy-ycguo · 2020-03-03T09:42:07Z

@rhuss @navidshaikh @maximilien I met with problem in this test case.

I have set up cronjob and event-display in the test environment. Yet there are error messages in cronjob adapter:

{"level":"error","ts":1583223540.0185604,"logger":"fallback","caller":"cronjobevents/adapter.go:120","msg":"failed to send cloudevent{error 25 0  Post http://displaysvc.kne2etests1.svc.cluster.local: dial tcp: lookup displaysvc.kne2etests1.svc.cluster.local on 10.11.240.10:53: no such host}","stacktrace":"knative.dev/eventing/pkg/adapter/cronjobevents.(*cronJobAdapter).cronTick\n\tknative.dev/eventing/pkg/adapter/cronjobevents/adapter.go:120\nknative.dev/eventing/vendor/github.com/robfig/cron.FuncJob.Run\n\tknative.dev/eventing/vendor/github.com/robfig/cron/cron.go:131\nknative.dev/eventing/vendor/github.com/robfig/cron.(*Cron).startJob.func1\n\tknative.dev/eventing/vendor/github.com/robfig/cron/cron.go:307"}

cronjob is trying to send the event messages to displaysvc.kne2etests1.svc.cluster.local but the host name could not be resolved. I think maybe we need to update istio install to use cluster local gateway in the test environment to solve the issue. Refer to this issue report in Knative eventing.

Please let me know your thoughts to the current situation. Thank you.

rhuss · 2020-03-03T14:46:15Z

test/e2e/cli.go

@@ -185,6 +185,7 @@ func RunKubectl(namespace string, args ...string) (string, error) {
 		args = append(args, "--namespace", namespace)
 	}
 	stdout, stderr, err := runCli("kubectl", args)
+	//out := fmt.Sprintf("$kubectl %s \n %s", args, stdout)


please remove

for debug purpose. will remove in the latest version.

rhuss · 2020-03-03T14:46:38Z

test/e2e/cronjob_test.go

@@ -56,7 +59,57 @@ func TestSourceCronJob(t *testing.T) {
 	jpSinkRefNameInSpec := "jsonpath={.spec.sink.ref.name}"
 	out, err := test.getResourceFieldsWithJSONPath("cronjobsource", "testcronjobsource2", jpSinkRefNameInSpec)
 	assert.NilError(t, err)
-	assert.Equal(t, out, "testsvc1")
+	//assert.Equal(t, out, "testsvc1")


please remove

@daisy-ycguo with the latest commits, there is a conflict.

wrt/ the issue: I will open an issue on test-infrastructure so that the istio local gateway gets enabled for the tests.

Until test-infrastructure gets update, this PR will not pass the integration test.

rhuss · 2020-03-03T14:47:50Z

test/e2e/cronjob_test.go

+	results := test.kn.Run("source", "cronjob", "describe", "testcronjobsource3")
+	r.AssertNoError(results)
+	out, err = kubectl{test.kn.namespace}.Run("get", "pod", "-l", "sources.eventing.knative.dev/cronJobSource=testcronjobsource3", "-o", "yaml")
+	t.Log(out)


Is this necessary ? Maybe better assert on out.

for debug purpose. will remove in the latest version.

rhuss · 2020-03-03T14:48:26Z

test/e2e/cronjob_test.go

+	for retries < 5 {
+		out, err = kubectl{test.kn.namespace}.Run("logs", "-l", selectorStr, "-c", "user-container")
+		if err != nil {
+			t.Logf("error happens at kubectl logs -l %s -c -n %s: %v", selectorStr, test.kn.namespace, err)


you should return here with the error.

rhuss · 2020-03-03T14:48:47Z

test/e2e/cronjob_test.go

+		out, err = kubectl{test.kn.namespace}.Run("logs", "-l", selectorStr, "-c", "user-container")
+		if err != nil {
+			t.Logf("error happens at kubectl logs -l %s -c -n %s: %v", selectorStr, test.kn.namespace, err)
+		} else if err == nil && strings.Contains(out, message) {


err is nil for sure when you reach this (see condition before)

rhuss · 2020-03-03T14:52:11Z

test/e2e/cronjob_test.go

+			break
+		} else {
+			t.Logf("return from kubectl logs -l %s -n %s: %s", selectorStr, test.kn.namespace, out)
+			out, err = kubectl{test.kn.namespace}.Run("get", "pods")


what if err ?

for debug purpose. will remove in the latest version.

rhuss · 2020-03-03T14:52:19Z

test/e2e/cronjob_test.go

+			t.Logf("return from kubectl logs -l %s -n %s: %s", selectorStr, test.kn.namespace, out)
+			out, err = kubectl{test.kn.namespace}.Run("get", "pods")
+			t.Log(out)
+			out, err = kubectl{test.kn.namespace}.Run("logs", "-l", "sources.eventing.knative.dev/cronJobSource=testcronjobsource3")


what if err ?

for debug purpose. will remove in the latest version.

rhuss · 2020-03-03T14:52:33Z

test/e2e/cronjob_test.go

+		} else {
+			t.Logf("return from kubectl logs -l %s -n %s: %s", selectorStr, test.kn.namespace, out)
+			out, err = kubectl{test.kn.namespace}.Run("get", "pods")
+			t.Log(out)


why log here ?

rhuss · 2020-03-03T14:52:56Z

test/e2e/cronjob_test.go

+			out, err = kubectl{test.kn.namespace}.Run("get", "pods")
+			t.Log(out)
+			out, err = kubectl{test.kn.namespace}.Run("logs", "-l", "sources.eventing.knative.dev/cronJobSource=testcronjobsource3")
+			t.Log(out)


for debug purpose. will remove in the latest version.

rhuss · 2020-03-03T14:53:13Z

test/e2e/cronjob_test.go

+
+	if retries == 5 {
+		return fmt.Errorf("Expected log incorrect after retry 5 times. Expecting to include:\n%s\n Instead found:\n%s\n", message, out)
+	} else {


else not needed

rhuss · 2020-03-04T09:52:55Z

@daisy-ycguo with the latest commits, there is a conflict.

wrt/ the issue: I will open an issue on test-infrastructure so that the istio local gateway gets enabled for the tests.

knative-prow-robot · 2020-03-04T09:53:12Z

@daisy-ycguo: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

rhuss · 2020-03-05T18:53:56Z

/hold

Sorry, but as we are moving to ping source as a replacement for a cronjob source we need to do it completely different.

maximilien · 2020-03-05T23:14:51Z

Hi, @daisy-ycguo, please mark comments that have been resolved by clicking "Resolve" this should make it easier to do subsequent reviews on previous one. Thanks.

rhuss · 2020-03-07T19:53:09Z

As mentioned we moved now to a PingSource. Could you please either update that PR, or, maybe simpler, create a new one for the PingSource ?

Thanks!

daisy-ycguo · 2020-03-09T08:46:51Z

@rhuss I will close it and raise a new one for the PingSource.

Co-authored-by: pratham-m <prathamm@us.ibm.com>

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

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 2, 2020

knative-prow-robot requested review from mattmoor and maximilien March 2, 2020 09:02

daisy-ycguo changed the title ~~Add cronjob event message verification to e2e test~~ Add event message verification to cronjob e2e test Mar 2, 2020

daisy-ycguo force-pushed the crone2e branch 6 times, most recently from 2455583 to c88470b Compare March 2, 2020 15:58

maximilien suggested changes Mar 2, 2020

View reviewed changes

daisy-ycguo force-pushed the crone2e branch 8 times, most recently from d1c0838 to 1275696 Compare March 3, 2020 07:32

add cronjob event message verification to e2e test

f564a3c

daisy-ycguo force-pushed the crone2e branch from 1275696 to f564a3c Compare March 3, 2020 08:08

rhuss requested changes Mar 3, 2020

View reviewed changes

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 4, 2020

rhuss mentioned this pull request Mar 4, 2020

How to enable Istio local gateway for Kn client tests ? knative/test-infra#1784

Closed

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 5, 2020

daisy-ycguo closed this Mar 9, 2020

dsimansk pushed a commit to dsimansk/client that referenced this pull request May 12, 2021

Fix for serviceaccounts "default" not found flaky issue (knative#698)

9b93ab3

Co-authored-by: pratham-m <prathamm@us.ibm.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add event message verification to cronjob e2e test #698

Add event message verification to cronjob e2e test #698

daisy-ycguo commented Mar 2, 2020

knative-prow-robot commented Mar 2, 2020

maximilien left a comment

maximilien Mar 2, 2020

daisy-ycguo Mar 3, 2020

maximilien Mar 2, 2020

daisy-ycguo Mar 3, 2020

maximilien Mar 2, 2020

maximilien Mar 2, 2020

daisy-ycguo Mar 3, 2020

knative-prow-robot commented Mar 3, 2020

daisy-ycguo commented Mar 3, 2020 •

edited

Loading

rhuss Mar 3, 2020

daisy-ycguo Mar 4, 2020 •

edited

Loading

rhuss Mar 3, 2020

daisy-ycguo Mar 4, 2020

rhuss Mar 3, 2020

daisy-ycguo Mar 4, 2020 •

edited

Loading

rhuss Mar 3, 2020

rhuss Mar 3, 2020

rhuss Mar 3, 2020

daisy-ycguo Mar 4, 2020

rhuss Mar 3, 2020

daisy-ycguo Mar 4, 2020

rhuss Mar 3, 2020

rhuss Mar 3, 2020

daisy-ycguo Mar 4, 2020

rhuss Mar 3, 2020

rhuss commented Mar 4, 2020

knative-prow-robot commented Mar 4, 2020

rhuss commented Mar 5, 2020

maximilien commented Mar 5, 2020

rhuss commented Mar 7, 2020

daisy-ycguo commented Mar 9, 2020

Add event message verification to cronjob e2e test #698

Add event message verification to cronjob e2e test #698

Conversation

daisy-ycguo commented Mar 2, 2020

Proposed Changes

knative-prow-robot commented Mar 2, 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

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

knative-prow-robot commented Mar 3, 2020

daisy-ycguo commented Mar 3, 2020 • edited Loading

Choose a reason for hiding this comment

daisy-ycguo Mar 4, 2020 • edited Loading

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

daisy-ycguo Mar 4, 2020 • edited Loading

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

rhuss commented Mar 4, 2020

knative-prow-robot commented Mar 4, 2020

rhuss commented Mar 5, 2020

maximilien commented Mar 5, 2020

rhuss commented Mar 7, 2020

daisy-ycguo commented Mar 9, 2020

daisy-ycguo commented Mar 3, 2020 •

edited

Loading

daisy-ycguo Mar 4, 2020 •

edited

Loading

daisy-ycguo Mar 4, 2020 •

edited

Loading