Skip to content
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

Accept CloudEvents in any order for CE reconcile test #3292

Merged
merged 1 commit into from Oct 5, 2020
Merged

Accept CloudEvents in any order for CE reconcile test #3292

merged 1 commit into from Oct 5, 2020

Conversation

ghost
Copy link

@ghost ghost commented Sep 28, 2020

Changes

Fixes #2992

Prior to this commit test cloud events received during reconcile were
required to be emitted in a strict order. We've been seeing some flakes in
our unit tests related to this and in
#2992 we posited that this might
be happening because the cloud events controller emits each event in a
separate go routine. We added some extra logging to try and catch the
details of the flakes and it does appear that the events are being sent
out-of-order.

This commit adds a new helper func that allows the expected cloudevents
to appear in any order while still failing the test if an unexpected event
(or not enough events) are received.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Release Notes

NONE

@ghost ghost added the kind/flake Categorizes issue or PR as related to a flakey test label Sep 28, 2020
@tekton-robot tekton-robot added the release-note-none Denotes a PR that doesnt merit a release note. label Sep 28, 2020
@tekton-robot tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 28, 2020
}
case <-timer.C:
if len(expected) != 0 {
return fmt.Errorf("timed out waiting for %d more events", len(expected))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe include the remaining expected events too

fmt.Errorf("timed out waiting for %d more events: %#v", len(expected), expected)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea; done!

@dibyom
Copy link
Member

dibyom commented Sep 28, 2020

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 28, 2020
Prior to this commit the test cloud events received during reconcile were
required to be emitted in a strict order. We've been seeing some flakes in
our unit tests related to this and in
#2992 we posited that this might
be happening because the cloud events controller emits each event in a
separate go routine. We added some extra logging to try and catch the
details of the flakes and it does appear that the events are being sent
out-of-order.

This commit adds a new helper func that allows the expected cloudevents
to appear in any order while still failing the test if an unexpected event
(or not enough events) are received.
@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 28, 2020
@ghost
Copy link
Author

ghost commented Sep 29, 2020

/test pull-tekton-pipeline-integration-tests

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/cc @afrittoli

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 29, 2020
@ghost ghost added this to the Pipelines v0.17 milestone Oct 2, 2020
Copy link
Member

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this!
I wonder if we should do the same thing in the pipeline run controller tests, but in any case this PR looks.

Copy link
Member

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@tekton-robot
Copy link
Collaborator

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 5, 2020
@ghost ghost removed the kind/flake Categorizes issue or PR as related to a flakey test label Oct 5, 2020
@ghost
Copy link
Author

ghost commented Oct 5, 2020

/kind flake

/retest

@tekton-robot tekton-robot added the kind/flake Categorizes issue or PR as related to a flakey test label Oct 5, 2020
@ghost
Copy link
Author

ghost commented Oct 5, 2020

/remove-kind flake

@tekton-robot tekton-robot removed the kind/flake Categorizes issue or PR as related to a flakey test label Oct 5, 2020
@ghost
Copy link
Author

ghost commented Oct 5, 2020

/kind flake

@tekton-robot tekton-robot added the kind/flake Categorizes issue or PR as related to a flakey test label Oct 5, 2020
@tekton-robot tekton-robot merged commit b16369e into tektoncd:master Oct 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/flake Categorizes issue or PR as related to a flakey test lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesnt merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TestReconcile_CloudEvents Unit test failed in nightly job
5 participants