EventTrasformationForTriggerTest can run with any Broker

[pierDipi]

Proposed Changes

• Allow running EventTrasformationForTriggerTest test with any Broker

[pierDipi]

/cc devguyio
/cc chizhg

[slinkydeveloper]

/lgtm
/approve
/hold

@devguyio mind giving a look? you're doing a similar thing in sources, so it would be better if we keep aligned the patterns, in order to reduce a bit the complexity of them 😄 unhold when it's ready for you

[vaikas]

Great! Thanks for working on this!!

Also related to:
#3531

My thinking was that for all these tests, we should just need to pass in the ns/broker name and run the tests against that. So pulling the Broker creation out. Ideally you could create the Broker out of band (kubectl) and just run these tests by specifying the ns/brokername.

Broker specific behaviour tests (creating them, are the right channels, etc. being created) should only really exercise the MT Channel Based Broker. Thoughts?

[slinkydeveloper]

Broker specific behaviour tests (creating them, are the right channels, etc. being created) should only really exercise the MT Channel Based Broker. Thoughts?

@vaikas I may be wrong, but i'm not sure this is always doable because some tests might want to configure some bits of the broker (not dependant on the specific version), like the dlq, and this seems to be better to properly configure it inside using the setup fn.

[pierDipi]

My thinking was that for all these tests, we should just need to pass in the ns/broker name and run the tests against that. So pulling the Broker creation out. Ideally you could create the Broker out of band (kubectl) and just run these tests by specifying the ns/brokername.

I actually think this is an interesting idea, not applicable to all tests, though.
For example, there are tests like control plane conformance tests where we assess specific conditions (e.g. ready / not ready) that we can reproduce when we create objects in a specific order.
Is the goal of this change improving debuggability?

Anyway, I think using a simple function like in this PR, nothing prevents us to create a no-op function that just returns the name of the object you created before, and for the namespace, we can tweak the client to use a specific namespace based on a flag. Of course, this needs to be extended not only to the Broker but also to the Trigger.

Broker specific behaviour tests (creating them, are the right channels, etc. being created) should only really exercise the MT Channel Based Broker. Thoughts?

@vaikas I may be wrong, but i'm not sure this is always doable because some tests might want to configure some bits of the broker (not dependant on the specific version), like the dlq, and this seems to be better to properly configure it inside using the setup fn.

My feeling is similar, there are tests that require a specific configuration of the object.
We can document this behavior and then it's up the user (us) to properly configure the object.

[vaikas]

Everybody's right but probably me :)

I think as you correctly point out there are few kinds of categories of tests where how the broker is created is important, the dlq example is a good one. But I think there are also classes of tests where we're testing things like routing, replying, and so forth where especially if I'm writing a broker, I'd like to run the tests against my broker that I have created, and I just think that by breaking the broker creation from the tests that exercise it might be helpful. Again, I think about this in terms:

• create the Unit Under Test
• Run tests

Right now if a user wants to not modify any of our test code but just use it, they would have to modify the tests. That can be a barrier for testing. I think it's reasonable to expect that if you run DLQ tests, you should configure your broker to be configured properly and so forth, or then I'd expect the tests to fail, which I think is a good thing :)

So @pierDipi you're totally right that nothing is preventing and as you point out you can pass in the BrokerCreator that just returns the string but that still requires changing our code. I wonder if we can just add (not part of this PR, this is great stuff you're doing here so no need to block on that) a flag that says use this broker instead, and then we can parse the flag and if it's given, we pass in a custom BrokerCreator that just returns that flag?

[vaikas]

/lgtm
/approve

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pierDipi, slinkydeveloper, vaikas

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,vaikas]

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

[slinkydeveloper]

/unhold

[pierDipi]

FAIL: TestPersistedStore (1.02s)

/retest