Closed pierDipi closed 5 months ago
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: pierDipi
The full list of commands accepted by this bot can be found here.
The pull request process is described here
I think it looks good. I will just give some time for any other reviewers. The only concern I have is that we already hold a slice of Features inside FeatureSet. I think FeatureSets could be used for this purpose as well like this:
e := feature.NewFeatureNamed("install eventshub")
e.Setup("install eventshub", eventshub.Install(subscriberName1, eventshub.StartReceiver))
b := feature.NewFeatureNamed("install brokers")
b.Setup("install broker1", broker.Install(...))
b.Setup("install broker2", broker.Install(...))
r := feature.NewFeatureNamed("ready brokers")
r.Requirement("ready broker1", broker.IsReady(...))
r.Requirement("ready broker2", broker.IsReady(...))
r.Assert("addressable broker1", broker.IsAddressable(...))
r.Assert("addressable broker2", broker.IsAddressable(...))
it := feature.NewFeatureNamed("install triggers")
it.Setup("install trigger1", trigger.Install(...))
it.Setup("install trigger2", trigger.Install(...))
rt := feature.NewFeatureNamed("ready triggers")
rt.Setup("ready trigger1", trigger.IsReady(...))
rt.Setup("ready trigger2", trigger.IsReady(...))
a := feature.NewFeatureNamed("assert feature set")
a.Assert(...)
fs := &feature.FeatureSet{
Name: "Multiple Brokers",
Features: []*feature.Feature{
e, b, r, it, rt, a,
},
}
env.TestSet(ctx, t, fs)
Individual features are tested sequentially. The fact that we install/configure multiple features like Brokers, Triggers, etc. it makes sense to use FeatureSet, IMO.
The other option (aside from using sub-features) would be something like this. Enforcing sequential execution of actions within a step, without compromising readability. Just putting it here for completeness: Additional type:
type StepFnSequence struct {
stepFunctions []StepFn
}
func (s *StepFnSequence) Add(fn StepFn) {
s.stepFunctions = append(s.stepFunctions, fn)
}
func (s *StepFnSequence) Run(ctx context.Context, t T) {
for _, fn := range s.stepFunctions {
fn(ctx, t)
}
}
Actual usage:
counterSeq := feature.StepFnSequence{}
counterSeq.Add(eventshub.Install(subscriberName1, eventshub.StartReceiver))
counterSeq.Add(broker.Install(...))
feat.Setup("setup1", counterSeq.Run)
/lgtm
This is a feature that might allow simplifying setting up tests to prevent starvation, for example, this one https://github.com/knative/eventing/pull/7911 while continuing to get the benefits of the parallel phases and avoiding having the error prone calls like
eventshub.Install(subscriberName2, eventshub.StartReceiver)(ctx, t)
See also usage example in
test/e2e/environment_test.go