Add source status conformance test and run it for ApiServerSource #3605

devguyio · 2020-07-16T13:24:18Z

Proposed Changes

Introduce component specific client options
Initialize ApiServerSource for the conformance tests
Add SourceStatusConformanceTest

Eventing conformance tests now can validate Sources status conformance

devguyio · 2020-07-16T13:27:33Z

@slinkydeveloper check out the test_runner.go and conformance/main_test.go

slinkydeveloper · 2020-07-16T13:59:00Z

test/conformance/main_test.go

@@ -38,10 +44,11 @@ func TestMain(m *testing.M) {
 			ComponentsToTest:    test.EventingFlags.Channels,
 		}
 		sourcesTestRunner = testlib.ComponentsTestRunner{
+			ComponentFeatureMap: testlib.SourceFeatureMap,


If from a project outside this repo (eg eventing-contrib) i want to run these conformance tests, i need to create this "main" file and populate that ComponentFeatureMap manually right?

kn conformance run ... ? Anyone?

Would it be possible to restructure this so that something like this:
https://github.com/knative/eventing/pull/3531/files#diff-9cd17936c8cf7a4c529c64712ffa54abR37

Since these are conformance tests it would be nice if the user doesn't have to manually modify the test code to test their source for conformance. Ideally the creation of the resource would be separate from the tests and for tests, all you'd need to do is to give a namespace/name to test and the test would run against it. I've been advocating for this pattern for all of the conformance tests to make them easier to test. I further believe this to be even more important for sources as there are going to be more sources than channels / brokers.

So it is my understanding that the "reusable" conformance tests package should not create any sources and should be "reusable" by other repos that want to run them against their sources, and that is the role of conformance/helpers package. Which is the case in this PR.

The repo specific tests that reuse the helpers (in our case test/conformance/main_tests.go & source_crd_metadata_test.go ) are the ones that inject the source creation tests. In eventing-contrib we'll do the same, inject the initializer that creates the KafkaSource etc and just reuse the conformance helper function.

@vaikas is the above different from what you want?

Well, Ideally I guess I'd like, just like today I can run E2E tests against my own cluster so I can push custom bits into it. I create my cluster, then ko apply my bits into it and then execute:
go test ./test/... -tags=e2e

I do not have to change any of the e2e tests to point to different cluster, nor have the tests have any knowledge how my bits got to the cluster.

Ideally what I think I'd like is the following experience for a user:
create their sources however they want (kubectl apply for example)
take the NS/Name of the above
then run something like:
go test ./test/... -tags=source-conformance -sourceNamespace=myns -sourceName=mysource
(or some shell script that executes that)

So, to run the tests I wouldn't have to modify the code at all. Maybe I'm missing why the helpers need to create these instead of having tests that take the namespace / name and execute the tests against it instead of creating the "units under test" in our repo being a requirement.

As agreed I'll do the cmd args in a separate PR

test/conformance/sources_conf_test.go

test/conformance/main_test.go

slinkydeveloper · 2020-07-16T14:10:02Z

test/lib/test_runner.go

+		features, present := tr.ComponentFeatureMap[component]
+		if !present || contains(features, feature) {
+			t.Run(fmt.Sprintf("%s-%s", component.Kind, component.APIVersion), func(st *testing.T) {
+				testFunc(st, component, tr.componentOptions[component]...)


Wait but you don't run the setup function here right? I lost where that function runs 😄

I totally feel ya 😞 . The idea is, since ComponentTestRunner is just a selector of components, it includes components and their related ClientSetupOptions, and when running the tests, it passes the component and its ClientSetupOptions to the test function since the test function has the env context (the client it needs to inject those ClientSetupOptions with). I'm not a big fan of how convoluted the code ends up but that's the tension I've identified before between the helpers and the assumptions they make on those helpers consumers.

If you have a cleaner way in mind to do this I'd gladly do it.

Ok so the setup function of the source is a ClientSetupOption, which is ran when the client is brought up right?

what really confuses me is that you're using the ClientSetupOption to setup the source. For me [T]Option fn in golang is a function that setups T, not that uses it... Maybe these setup functions should be run as "post client setup" functions?

test/conformance/main_test.go

devguyio · 2020-07-22T07:15:58Z

/retest

devguyio · 2020-07-22T07:50:27Z

/retest

devguyio · 2020-07-22T08:05:13Z

/test pull-knative-eventing-unit-tests

devguyio · 2020-07-22T09:00:46Z

/retest

devguyio · 2020-07-22T11:26:45Z

/retest

devguyio · 2020-07-22T12:42:23Z

/retest

devguyio · 2020-07-22T14:27:42Z

/retest

Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>

aliok · 2020-07-24T11:55:25Z

@devguyio do we want to merge this one and do the refactoring later or create the structure as per the discussions above?

matzew · 2020-07-24T11:56:40Z

@devguyio can you rebase ?

devguyio · 2020-07-24T14:21:19Z

/retest

devguyio · 2020-07-24T15:30:53Z

/retest

aliok

/lgtm
/approve

Let's move forward with this IMO and then we can refactor.

slinkydeveloper · 2020-07-27T08:07:24Z

/lgtm
/approve

knative-prow-robot · 2020-07-27T08:07:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aliok, devguyio, slinkydeveloper

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:

~~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

knative-test-reporter-robot · 2020-07-27T08:51:17Z

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.TestEventTransformationForTriggerV1BrokerV1
test/e2e.TestEventTransformationForTriggerV1BrokerV1/InMemoryChannel-messaging.knative.dev/v1
test/e2e.TestEventTransformationForTriggerV1Beta1BrokerV1/Channel-messaging.knative.dev/v1beta1

pierDipi · 2020-07-27T08:53:19Z

Internal error occurred: failed calling webhook "webhook.eventing.knative.dev": Post https://eventing-webhook.knative-eventing-b7hnzjxjsb.svc:443/defaulting?timeout=2s: EOF

related: knative/pkg#1509
/retest

devguyio · 2020-07-27T09:05:21Z

/uncc @yt3liu
/uncc @chizhg

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 Jul 16, 2020

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

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. area/test-and-release Test infrastructure, tests or release labels Jul 16, 2020

knative-prow-robot requested review from chizhg and yt3liu July 16, 2020 13:24

slinkydeveloper reviewed Jul 16, 2020

View reviewed changes

aliok reviewed Jul 17, 2020

View reviewed changes

test/conformance/main_test.go Outdated Show resolved Hide resolved

devguyio force-pushed the 3117-src-status-conf branch from d0efc76 to 87ebaf6 Compare July 22, 2020 06:45

devguyio changed the title ~~[WIP] Introduce component specific client options~~ Add source status conformance test and run it for ApiServerSource Jul 22, 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 Jul 22, 2020

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 22, 2020

devguyio mentioned this pull request Jul 22, 2020

Add PingSource conformance tests initialization #3680

Merged

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

devguyio added 5 commits July 24, 2020 10:26

Initial dirty WIP code

de837c1

Add proper ApiServerSource initializer and finish status conf test

c516910

Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>

Remove an early recordevent cleanup

faa59af

Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>

Update deps with update-codegen.sh result

9e21638

Run e2e-tests and conformance against v1beta1 ApiServerSource

37e16e6

Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>

devguyio force-pushed the 3117-src-status-conf branch from 8dd333a to 37e16e6 Compare July 24, 2020 14:01

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

Add missing e2e build tag

22128cc

aliok approved these changes Jul 27, 2020

View reviewed changes

knative-prow-robot assigned aliok Jul 27, 2020

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

knative-prow-robot assigned slinkydeveloper Jul 27, 2020

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

knative-prow-robot removed request for yt3liu and chizhg July 27, 2020 09:05

knative-prow-robot merged commit a471461 into knative:master Jul 27, 2020

This was referenced Jul 28, 2020

Source conformance tests - CRD validation #3117

Closed

Properly configure conformance tests to run source status conformance test against KafkaSource knative/eventing-contrib#1410

Closed

aliok mentioned this pull request Sep 25, 2020

Add aliok to approvers #4118

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add source status conformance test and run it for ApiServerSource #3605

Add source status conformance test and run it for ApiServerSource #3605

devguyio commented Jul 16, 2020 •

edited

Loading

devguyio commented Jul 16, 2020 •

edited

Loading

slinkydeveloper Jul 16, 2020

devguyio Jul 16, 2020

lionelvillard Jul 22, 2020

vaikas Jul 22, 2020

devguyio Jul 22, 2020

vaikas Jul 23, 2020

devguyio Jul 24, 2020

slinkydeveloper Jul 16, 2020

devguyio Jul 16, 2020

slinkydeveloper Jul 16, 2020

slinkydeveloper Jul 16, 2020

devguyio commented Jul 22, 2020

devguyio commented Jul 22, 2020

devguyio commented Jul 22, 2020

devguyio commented Jul 22, 2020

devguyio commented Jul 22, 2020

devguyio commented Jul 22, 2020

devguyio commented Jul 22, 2020

aliok commented Jul 24, 2020

matzew commented Jul 24, 2020

devguyio commented Jul 24, 2020

devguyio commented Jul 24, 2020

aliok left a comment

slinkydeveloper commented Jul 27, 2020

knative-prow-robot commented Jul 27, 2020

knative-test-reporter-robot commented Jul 27, 2020

pierDipi commented Jul 27, 2020

devguyio commented Jul 27, 2020

Add source status conformance test and run it for ApiServerSource #3605

Add source status conformance test and run it for ApiServerSource #3605

Conversation

devguyio commented Jul 16, 2020 • edited Loading

Proposed Changes

devguyio commented Jul 16, 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

devguyio commented Jul 22, 2020

devguyio commented Jul 22, 2020

devguyio commented Jul 22, 2020

devguyio commented Jul 22, 2020

devguyio commented Jul 22, 2020

devguyio commented Jul 22, 2020

devguyio commented Jul 22, 2020

aliok commented Jul 24, 2020

matzew commented Jul 24, 2020

devguyio commented Jul 24, 2020

devguyio commented Jul 24, 2020

aliok left a comment

Choose a reason for hiding this comment

slinkydeveloper commented Jul 27, 2020

knative-prow-robot commented Jul 27, 2020

knative-test-reporter-robot commented Jul 27, 2020

pierDipi commented Jul 27, 2020

devguyio commented Jul 27, 2020

devguyio commented Jul 16, 2020 •

edited

Loading

devguyio commented Jul 16, 2020 •

edited

Loading