Removed all usages of old asserts and using the new StartEventRecord by slinkydeveloper · Pull Request #3302 · knative/eventing

slinkydeveloper · 2020-06-10T09:18:45Z

Signed-off-by: Francesco Guardiani francescoguard@gmail.com

Part of #3267

Proposed Changes

Removed all usages and methods of AssertWaitMatchSourceData
When possible, removed manual event record creation and used recordevents.StartEventRecordOrFail(client, recordEventsPodName)

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

knative-prow-robot · 2020-06-10T09:18:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: slinkydeveloper

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~test/OWNERS~~ [slinkydeveloper]

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

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

slinkydeveloper · 2020-06-10T09:20:01Z

cc @nlopezgi

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

knative-test-reporter-robot · 2020-06-10T10:06:08Z

The following jobs failed:

Test name	Triggers	Retries
pull-knative-eventing-integration-tests		0/3

Failed non-flaky tests preventing automatic retry of pull-knative-eventing-integration-tests:

test/e2e.TestParallel
test/e2e.TestSequence
test/e2e.TestApiServerSource
test/e2e.TestApiServerSource/event-ref
test/e2e.TestApiServerSource/event-ref-unmatch-label
test/e2e.TestApiServerSource/event-ref-match-label
test/e2e.TestApiServerSource/event-ref-match-label-expr
test/e2e.TestSequence/Channel-messaging.knative.dev/v1beta1

and 3 more.

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

nlopezgi · 2020-06-10T13:59:58Z

-			if len(ev) != 0 {
-				t.Fatalf("Log is not empty in logger pod %q: %d events seen", loggerPodName, len(ev))
+		t.Run(tc.name, func(t *testing.T) {
+			tc := tc


is this line needed?

Look here: https://blog.golang.org/subtests#run-a-group-of-tests-in-parallel

nlopezgi · 2020-06-10T15:03:48Z

+			client := setup(t, true)
+			defer tearDown(client)
+
+			// creates ServiceAccount and RoleBinding with a role for reading pods and events


So I understand, why did you pull this code from outside the loop to in here (might be a golang noob q, apologies if so)?

Because the client setup in this way setup a context for each test, allowing the tests to run in parallel (before it had some race issues)

nlopezgi · 2020-06-10T15:13:44Z

-		t.Fatalf("Data %s, extension %q does not appear at least %d times in events of logger pod %q: %v", data, extensionSecret, expectedCount, loggerPodName, err)
-
-	}
+	eventTracker.AssertAtLeast(2, recordevents.MatchEvent(


One thing I have seen when using these assertions, is that the error messaging could be improved (not something that needs to happen in this CL, just commenting about it).
Specifically, I get messages like:
Expected messages not found: FAIL MATCHING: saw 0/1 matching events. recent events: (1 events seen, last N = [{ ... }]
The message shows me what events were observed, and that they failed to match 'something', but it does not tell me what it was actually matching for, why the events(s) did not match, or even where in the code to look for the matching. I ended up having to modify the matcher so that it printed to console more details.

Agree and that should have improved it a bit https://github.com/knative/eventing/pull/3302/files#diff-719035a5ad84fc383e0dea6ef4c67e32R267

I'm not a fan of "logging" the matcher, because the code already describes it and it requires quite some work to log the matcher as string.
On the other hand, i don't get why I can't see the stacktrace that brings me to where the assertion is coded

Though I agree with the feeling that logging the matcher is not ideal, the situation for debugging is not good as it is. Check for instance the matcher here: https://github.com/knative/eventing/blob/master/test/conformance/helpers/channel_tracing_test_helper.go#L341
I was not able to ever find out where the event that is passed to that match function is created. Could you help me locate it and help me understand how one goes from the matcher code, to the event instance, in a way that makes for this logging not strictly necessary.

I was not able to ever find out where the event that is passed to that match function is created. Could you help me locate it and help me understand how one goes from the matcher code, to the event instance, in a way that makes for this logging not strictly necessary.

Ok TBH I don't get what you mean. How about getting this one in, see if the situation improved and then open a new issue to make it better? With this one we print the match failures, so that should be a beginning. What I wish now is to print the line where the Assert is invoked

yeah, sgtm, my comment definitely does not block this PR.

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

nlopezgi · 2020-06-10T20:01:03Z

/lgtm

Removed all usages of old asserts and using the new StartEventRecord

55d7edf

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

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

knative-prow-robot requested review from steuhs and yt3liu June 10, 2020 09:18

knative-prow-robot added area/test-and-release Test infrastructure, tests or release approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 10, 2020

slinkydeveloper mentioned this pull request Jun 10, 2020

Port e2e tests to new event sender and event record images #3267

Closed

17 tasks

mattmoor reviewed Jun 10, 2020

View reviewed changes

Comment thread test/e2e/source_api_server_test.go Outdated

Removed old imports

aac1053

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

slinkydeveloper added 2 commits June 10, 2020 12:42

Wrong data race

7ec469f

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

Incresed verbosity when not matching

ca07704

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

nlopezgi reviewed Jun 10, 2020

View reviewed changes

Applied suggestions

3e7c4fc

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>

knative-prow-robot assigned nlopezgi Jun 10, 2020

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

knative-prow-robot merged commit c965f86 into knative:master Jun 10, 2020

slinkydeveloper deleted the remove_usage_deprecated_methods branch June 10, 2020 20:25

Conversation

slinkydeveloper commented Jun 10, 2020

Proposed Changes

Uh oh!

knative-prow-robot commented Jun 10, 2020

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

slinkydeveloper commented Jun 10, 2020

Uh oh!

knative-test-reporter-robot commented Jun 10, 2020

Uh oh!

nlopezgi Jun 10, 2020

Choose a reason for hiding this comment

Uh oh!

slinkydeveloper Jun 10, 2020

Choose a reason for hiding this comment

Uh oh!

nlopezgi Jun 10, 2020

Choose a reason for hiding this comment

Uh oh!

slinkydeveloper Jun 10, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nlopezgi Jun 10, 2020

Choose a reason for hiding this comment

Uh oh!

slinkydeveloper Jun 10, 2020 • 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!

nlopezgi Jun 10, 2020

Choose a reason for hiding this comment

Uh oh!

slinkydeveloper Jun 10, 2020 • 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!

nlopezgi Jun 10, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nlopezgi commented Jun 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

slinkydeveloper Jun 10, 2020 •

edited

Loading

slinkydeveloper Jun 10, 2020 •

edited

Loading