-
Notifications
You must be signed in to change notification settings - Fork 23
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
Netobserv 390: fix kafka transformer #237
Conversation
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'd add unit tests or integration tests, 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'd add a test that actually verifies the current additions. Something like:
- Abstract current kafka listener behind an injectable interface to provide mocks/fakes.
Test 1:
- Set a very high batch read timeout (e.g. 1h)
- Send mocked kafka messages with a total size lower than batchMaxLength
- Verify that messages aren't forwarded by the output channel
- Send messages until the total size is higher than batchMaxLength
- Verify that messages are forwarded by the output channel
Test 2:
- Set a very high batchMaxlength
- set a reasonably low batch timeout: e.g. 100 ms
- Send few mocked kafka messages (size < batchMatLength), making sure they are received before these 100ms. If we can't, increase the timeout e.g. to 200ms or ideally, also allow injecting a ticker that can be triggered on demand.
- Verify that the kafka messages are sent after the timeout
237d589
to
d90a2e4
Compare
Codecov Report
@@ Coverage Diff @@
## main #237 +/- ##
==========================================
+ Coverage 61.22% 61.32% +0.10%
==========================================
Files 67 67
Lines 3863 3886 +23
==========================================
+ Hits 2365 2383 +18
- Misses 1348 1352 +4
- Partials 150 151 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Such tests could be added to test/e2e/kafka/kafka_test.go or the existing tests could be adjusted to include these. |
I added the suggested tests. |
Multiple small changes in this PR: