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

job,test: Improved reliability of TestTimer_DoubleTrigger #26022

Conversation

dylandreimerink
Copy link
Member

There existed an edge case in this test which caused it to flake:

panic: close of closed channel

goroutine 106 [running]:
github.com/cilium/cilium/pkg/hive/job.TestTimer_DoubleTrigger.func1()
	/home/runner/work/cilium/cilium/pkg/hive/job/job_test.go:445 +0x1d

This PR moves the triggering of the timer to before the hive starts to ensure they are all processes before the timer has a chance to start consuming them. Also added a guard around the closing of the channel so we don't panic if the test were to fail for another reason.

Improved reliability of pkg/hive/job timer double trigger unit test

@dylandreimerink dylandreimerink added release-note/ci This PR makes changes to the CI. ci/flake This is a known failure that occurs in the tree. Please investigate me! labels Jun 8, 2023
@dylandreimerink dylandreimerink requested a review from a team as a code owner June 8, 2023 09:58
Copy link
Contributor

@ti-mo ti-mo left a comment

Choose a reason for hiding this comment

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

Thanks! One concern around using time.Sleep().

pkg/hive/job/job_test.go Outdated Show resolved Hide resolved
There existed an edge case in this test which caused it to flake:
```
panic: close of closed channel

goroutine 106 [running]:
github.com/cilium/cilium/pkg/hive/job.TestTimer_DoubleTrigger.func1()
	/home/runner/work/cilium/cilium/pkg/hive/job/job_test.go:445 +0x1d
```

This commit moves the triggering of the timer to before the hive starts
to ensure they are all processes before the timer has a chance to
start consuming them. Also added a guard around the closing of the
channel so we don't panic if the test were to fail for another reason.

Signed-off-by: Dylan Reimerink <dylan.reimerink@isovalent.com>
@dylandreimerink dylandreimerink force-pushed the feature/job-fix-double-trigger-flake branch from 32a8c30 to 8352c44 Compare June 8, 2023 10:14
@dylandreimerink
Copy link
Member Author

/test

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 8, 2023
@dylandreimerink dylandreimerink merged commit 441548e into cilium:main Jun 8, 2023
63 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/flake This is a known failure that occurs in the tree. Please investigate me! ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/ci This PR makes changes to the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants