-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor CloudEvent related tests to use common set of helpers #4387
Conversation
698dc3f
to
8f2e745
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
/remove-kind cleanup |
/kind cleanup |
/remove-kind cleanup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, nice, thank you for this!
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: afrittoli 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 |
The following is the coverage report on the affected files.
|
|
||
// CheckEvents checks that the events received by the FakeRecorder are the same as wantEvents, | ||
// in the same order. | ||
func CheckEvents(t *testing.T, fr *record.FakeRecorder, testName string, wantEvents []string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just realised that cloudevents.CheckEvents
is not checking cloudevents, which seems like a bit of a misnomer given the package's name. Suggest moving this func to the pkg/reconciler/events
package if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that ideally we'd have CheckEvents in the events package and CheckCloudEvents in the cloudevents package. However it looks like the events package is already doing some cloudevent related things and relies on checkCloudEvents in tests, which would cause a circular import if I split up tests in this way.
It seems like the events package may just need to be refactored but that's probably out of scope for this PR-- I'll create an issue to track.
Prior to this change, the event-related testing helpers were copied and pasted in several files. The TaskRun Reconcile tests were updated to not expect cloud events in a particular order, reducing flakiness, but the PipelineRun reconcile tests and the Event tests did not receive the same updates. This commit moves event-related test helpers into a common package and updates all CloudEvent tests to not expect events to occur in a given order. This should address some of the causes of flakes in tektoncd#2992 but not all. Co-authored-by: Scott Seaward [email protected]
The following is the coverage report on the affected files.
|
/lgtm |
@bobcatfish Sure thing, moved under |
Prior to this change, the event-related testing helpers were copied
and pasted in several files. The TaskRun Reconcile tests were updated
to not expect cloud events in a particular order, reducing flakiness,
but the PipelineRun reconcile tests and the Event tests did not receive
the same updates.
This commit moves event-related test helpers into a common package
and updates all CloudEvent tests to not expect events to occur in a given order.
This should address some of the causes of flakes in #2992 but not all.
Co-authored-by: Scott Seaward [email protected]
Changes
/kind cleanup
/kind flake
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
Release Notes