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

test(kafka): Added additional test for kafka startup logic #14391

Merged
merged 2 commits into from
Oct 4, 2024

Conversation

benclive
Copy link
Contributor

@benclive benclive commented Oct 4, 2024

What this PR does / why we need it:

Adds a test for the processNextFetchesUntilLagHonored function which does quite a lot of logic and I felt was undertested currently.

  • The test checks that the retries happen as expected & that kafka is called the expected number of times to replay any pending data.
  • I inject the delays by providing a custom time.Since function, which is when retries are determined. The production implementation just uses the real time.Since.
  • The test itself is relatively complicated, with several goroutines, so while I think it is correct, I added a timeout context. I'm open to simply closing this PR and skipping this test if the value is not worth the test complexity.

@benclive benclive requested a review from a team as a code owner October 4, 2024 09:56
@benclive benclive changed the title rewrite using events instead of time fix(kafka): Added additional test for kafka startup logic Oct 4, 2024
@benclive benclive changed the title fix(kafka): Added additional test for kafka startup logic test(kafka): Added additional test for kafka startup logic Oct 4, 2024
Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks for coming back to this and make sure we have a good test.

@cyriltovena cyriltovena merged commit c76ff14 into main Oct 4, 2024
60 checks passed
@cyriltovena cyriltovena deleted the benclive/add-extra-kafka-startup-logic-test branch October 4, 2024 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants