Add Regression test for large number of Triggers #449
Conversation
tekton-robot
added
the
do-not-merge/work-in-progress
tekton-robot
added
the
size/M
Fixes tektoncd#405 Issue tektoncd#356 found a bug in Triggers v0.1.0 (fixed in v0.2.0) where the admission webhook times out after adding around 35 Triggers to an EventListener. This PR adds a regression test where we create an EventListener with 1000 Triggers. Also, moves the cluster-scope resource cleanup code (from tektoncd@7ba80a7) out of init_test.go and into eventlistener_test.go.
tekton-robot
removed
the
do-not-merge/work-in-progress
| @@ -117,6 +117,13 @@ be used to run only [the unit tests](#unit-tests), i.e.: | |||
| // +build e2e | |||
| ``` | |||
|
|
|||
| #### Cleaning up cluster-scoped resources | |||
|
|
|||
| Each integration test runs in its own Namespace; each Namespace is torn down | |||
| } | ||
|
|
||
| // Verify that the EventListener was created properly | ||
| if err := WaitFor(eventListenerReady(t, c, namespace, el.Name)); err != nil { |
There was a problem hiding this comment.
Do we need to verify that the EL actually contains the 1000 triggers?
There was a problem hiding this comment.
Or perhaps that the sink got created?
There was a problem hiding this comment.
This should verify that the sink got created, right? Because it waits for the sink Pod to be ready.
As for verifying that the EventListener actually contains the 1000 Triggers... I guess we could trigger an event to fire all of the Triggers (maybe create a TaskRun or something?). I don't know if this would stress out our test cluster though 😅
Or do you mean just checking the length of the Triggers array in the EventListener returned after creation?
There was a problem hiding this comment.
ahh you are right...I did not notice that it actually waits for the sink to be created
There was a problem hiding this comment.
What about verifying the 1000 Triggers? Do you think it's alright without that? (I assume yes, since you merged the PR 😄)
There was a problem hiding this comment.
Yes, we have other tests that verify that we support multiple triggers
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dibyom 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:
Approvers can indicate their approval by writing |
tekton-robot
added
the
approved
|
/lgtm |
Changes
Fixes #405
Issue #356 found a bug in Triggers v0.1.0 (fixed in v0.2.0) where the
admission webhook times out after adding around 35 Triggers to an
EventListener.
This PR adds a regression test where we create an EventListener with
1000 Triggers.
I also moved the clean up code for cluster-scoped resources (from 7ba80a7) out of
init_test.goand intoeventlistener_test.go, because the code was cleaning up resources specific to theeventlistener_test.go. @dibyom do you think I should put this in a separate commit, or is it ok as it is?Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.