Event type test logical error by Leo6Leo · Pull Request #7166 · knative/eventing

Leo6Leo · 2023-08-10T17:53:46Z

Proposed Changes

Rework with the logic to be

for each event type in the list:
  - remove it from expectedCeTypes (ideally after cloning the object to not change the caller object)

assert that expectedCeTypes is empty

And also add the support to enable the auto eventType creation. Currently PingSource doesn't support the auto eventType Creation. We are enable the broker side auto eventType creation for now. Otherwise, the test will be failing.

Pre-review Checklist

At least 80% unit test coverage
E2E tests for any new behavior
Docs PR for any user-facing impact
Spec PR for any new API feature
Conformance test for any change to the spec

Signed-off-by: Leo Li <leoli@redhat.com>

knative-prow · 2023-08-10T17:53:57Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Signed-off-by: Leo Li <leoli@redhat.com>

codecov · 2023-08-10T18:04:43Z

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (25400fb) 77.73% compared to head (ecb2265) 77.73%.
Report is 40 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #7166   +/-   ##
=======================================
  Coverage   77.73%   77.73%           
=======================================
  Files         246      246           
  Lines       13284    13284           
=======================================
  Hits        10326    10326           
  Misses       2435     2435           
  Partials      523      523

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Leo Li <leoli@redhat.com>

Leo6Leo · 2023-08-10T19:08:53Z

/cc @creydr @matzew @Cali0707
Ready for review

Cali0707 · 2023-08-10T19:21:00Z

+	// Enable the eventtype auto-creation in configmap
+	f.Setup("enable eventtype auto-creation", eventtype.ApplyEventTypeConfigMap())
+


I'm not sure we should be enabling experimental features in the rekt (non-experimental) tests. Maybe if the events are not created by the ping source it makes more sense to remove the assertion rather than enabling this?

Co-authored-by: Calum Murray <cmurray@redhat.com>

Signed-off-by: Leo Li <leoli@redhat.com>

Leo6Leo · 2023-08-14T14:31:02Z

#7165 (comment)

As Knative documentation said, Support for automatic registration is not supported for PingSource.
Should we add the support for PingSource to have auto eventType creation? Or we will enable the broker-side eventType auto creation for PingSource's rekt test?

So for this ticket, any suggestions on what should we do next? Should we add the support for PingSource to have auto eventType creation? @creydr

creydr · 2023-08-15T08:58:38Z

#7165 (comment)

As Knative documentation said, Support for automatic registration is not supported for PingSource.
Should we add the support for PingSource to have auto eventType creation? Or we will enable the broker-side eventType auto creation for PingSource's rekt test?

So for this ticket, any suggestions on what should we do next? Should we add the support for PingSource to have auto eventType creation? @creydr

I agree with @Cali0707 in https://github.com/knative/eventing/pull/7166/files#r1290583414. We should not enable experimental features at this place.
For this PR to fix this logic (as the current logic is wrong), I would move this test to the experimental tests and then (when we have the support for auto event type creation in the pingsource) move it to the regular suite back (IMO).
That way, we can reuse the eventtype.WaitForEventType() and eventtype.AssertPresent() in other tests with the correct logic.

Signed-off-by: Leo Li <leoli@redhat.com>

Leo6Leo · 2023-08-25T18:33:49Z

/cc @Cali0707

Leo6Leo · 2023-08-28T14:29:02Z

/unhold Ready for review

…metal dir Signed-off-by: Leo Li <leoli@redhat.com>

Leo6Leo · 2023-08-28T15:06:48Z

/hold clean everything up

Signed-off-by: Leo Li <leoli@redhat.com>

Leo6Leo · 2023-09-05T18:55:59Z

/unhold

Cali0707

/lgtm

Thanks @Leo6Leo!

Cali0707 · 2023-09-05T19:36:23Z

@Leo6Leo I just realized the reconciler tests are still failing for this PR. Could you try and fix those?

pierDipi

/lgtm
/approve

knative-prow · 2023-10-02T13:49:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cali0707, Leo6Leo, pierDipi

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

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

Leo6Leo · 2023-10-02T18:40:55Z

/retest-required

Leo6Leo added 2 commits August 10, 2023 13:19

Fix the merge conflict and fix the logical error in the test

c77ee3d

Signed-off-by: Leo Li <leoli@redhat.com>

Add the auto eventType creation support

605a337

Signed-off-by: Leo Li <leoli@redhat.com>

knative-prow Bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 10, 2023

knative-prow Bot requested review from evankanderson and mgencur August 10, 2023 17:54

knative-prow Bot added the area/test-and-release Test infrastructure, tests or release label Aug 10, 2023

Leo6Leo added 2 commits August 10, 2023 13:58

reformat and remove the comments

cd60211

Signed-off-by: Leo Li <leoli@redhat.com>

Remove the assertReference function

e58d700

Signed-off-by: Leo Li <leoli@redhat.com>

fix reviewDog issue

adcdff7

Signed-off-by: Leo Li <leoli@redhat.com>

Leo6Leo marked this pull request as ready for review August 10, 2023 18:53

knative-prow Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 10, 2023

knative-prow Bot requested review from Cali0707, creydr and matzew August 10, 2023 19:12

Cali0707 requested changes Aug 10, 2023

View reviewed changes

Update test/rekt/resources/eventtype/eventtype.go

8e4150e

Co-authored-by: Calum Murray <cmurray@redhat.com>

Leo6Leo mentioned this pull request Aug 10, 2023

Remove the deprecated string type #7167

Closed

knative-prow Bot assigned Cali0707 Aug 10, 2023

fix reviewDog issue

9a4200e

Signed-off-by: Leo Li <leoli@redhat.com>

Merge branch 'main' into eventType-test-logical-error

5783715

Leo6Leo force-pushed the eventType-test-logical-error branch from ce97b63 to 5783715 Compare August 14, 2023 15:08

Merge branch 'main' into eventType-test-logical-error

a700867

This was referenced Aug 15, 2023

Move eventtype rekt test for pingsource to experimental rekt tests #7181

Closed

Move eventtype rekttest pingsource to experiment #7180

Merged

Merge the main branch

b688572

Signed-off-by: Leo Li <leoli@redhat.com>

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 25, 2023

knative-prow Bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 25, 2023

matzew reviewed Aug 28, 2023

View reviewed changes

Comment thread test/rekt/apiserversource_test.go Outdated

matzew reviewed Aug 28, 2023

View reviewed changes

Comment thread test/rekt/features/pingsource/features.go

Merge branch 'main' into eventType-test-logical-error

5861a71

matzew reviewed Aug 28, 2023

View reviewed changes

Comment thread test/rekt/pingsource_test.go Outdated

Remove the event type test from here, that has been moved to experien…

6888f63

…metal dir Signed-off-by: Leo Li <leoli@redhat.com>

knative-prow Bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 28, 2023

Clean up the mess

74748c6

Signed-off-by: Leo Li <leoli@redhat.com>

knative-prow Bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 28, 2023

Merge branch 'main' into eventType-test-logical-error

ecb2265

Leo6Leo requested a review from matzew September 5, 2023 18:55

knative-prow Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 5, 2023

Cali0707 approved these changes Sep 5, 2023

View reviewed changes

knative-prow Bot added the lgtm Indicates that a PR is ready to be merged. label Sep 5, 2023

pierDipi approved these changes Oct 2, 2023

View reviewed changes

knative-prow Bot assigned pierDipi Oct 2, 2023

knative-prow Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 2, 2023

knative-prow Bot merged commit 69dbad2 into knative:main Oct 2, 2023

		// Enable the eventtype auto-creation in configmap
		f.Setup("enable eventtype auto-creation", eventtype.ApplyEventTypeConfigMap())

Conversation

Leo6Leo commented Aug 10, 2023

Proposed Changes

Pre-review Checklist

Uh oh!

knative-prow Bot commented Aug 10, 2023

Uh oh!

codecov Bot commented Aug 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Leo6Leo commented Aug 10, 2023

Uh oh!

Uh oh!

Cali0707 Aug 10, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Leo6Leo commented Aug 14, 2023

Uh oh!

creydr commented Aug 15, 2023

Uh oh!

Leo6Leo commented Aug 25, 2023

Uh oh!

Leo6Leo commented Aug 28, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Leo6Leo commented Aug 28, 2023

Uh oh!

Leo6Leo commented Sep 5, 2023

Uh oh!

Cali0707 left a comment

Choose a reason for hiding this comment

Uh oh!

Cali0707 commented Sep 5, 2023

Uh oh!

pierDipi left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow Bot commented Oct 2, 2023

Uh oh!

Leo6Leo commented Oct 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov Bot commented Aug 10, 2023 •

edited

Loading