Add tests for dlq_handler.go #6071
Conversation
fimanishi
force-pushed
the
tests-for-dlq_handler
branch
2 times, most recently
from
6c754a8
to
767144e
Compare
What changed? tests for dlq_handler.go. Moved getInterval function to dlqHandleImpl to allow testing Why? improve unit test coverage How did you test it? unit tests Potential risks Release notes Documentation Changes
fimanishi
force-pushed
the
tests-for-dlq_handler
branch
from
767144e
to
2bf9e69
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files
... and 49 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Pull Request Test Coverage Report for Build 018fdeff-1562-4c21-8049-dfdd6139a4eeDetails
💛 - Coveralls |
fimanishi
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
Groxx,
shijiesheng,
agautam478,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
|
|
||
| for _, tc := range tests { | ||
| s.T().Run(tc.name, func(t *testing.T) { | ||
| s.messageHandler.status = tc.status |
There was a problem hiding this comment.
let's also validate no goroutines leaked after stopping.
defer goleak.VerifyNone(t)
| } | ||
| } | ||
|
|
||
| func (s *dlqHandlerSuite) TestStop() { |
There was a problem hiding this comment.
what is the point of this test given that above already covers start/stop flow?
|
|
||
| func (s *dlqHandlerSuite) TestEmitDLQSizeMetricsLoop_StopsWhenDoneSignalReceived() { | ||
| s.messageHandler.status = common.DaemonStatusStarted | ||
| go s.messageHandler.emitDLQSizeMetricsLoop() |
There was a problem hiding this comment.
validate these goroutines are terminated via defer goleak.VerifyNone(t)
|
|
||
| func (s *dlqHandlerSuite) TestEmitDLQSizeMetricsLoop_FetchesAndEmitsMetricsPeriodically() { | ||
| emissionNumber := 2 | ||
| interval := 1 * time.Second |
There was a problem hiding this comment.
you can lower this to 100ms or something to lower the overall test run time
There was a problem hiding this comment.
Made an attempt to use timeSource and Advance. Let me know what you think. Had to make some changes to the methods to avoid using sleep, as suggested by @3vilhamster
| } | ||
|
|
||
| timer := time.NewTimer(getInterval()) | ||
| timer := time.NewTimer(r.getInterval()) |
There was a problem hiding this comment.
I recommend using clock.TimeSource to mimic time-related behaviour.
It allows to have the same behavior for prod and be able to mimic time changes in tests without any time delays.
There was a problem hiding this comment.
Good point. I've made and attempt to fix that. Unfortunately the timer was set inside of the method being called by the goroutine, so I had to make further changes in order to be able to use Advance without using sleep. Let me know what you think.
…use timeSource to improve testability
fimanishi
force-pushed
the
tests-for-dlq_handler
branch
from
82f89de
to
867b94e
Compare
|
|
||
| go s.messageHandler.emitDLQSizeMetricsLoop(timer) | ||
|
|
||
| // Advance time to trigger the next emission |
There was a problem hiding this comment.
Thank you for doing that. It looks better now.
However, I recommend adding a call
mockTimeSource.BlockUntil(1)
To ensure the goroutine was started and is blocked on the awaiting timer.
There was a problem hiding this comment.
This is awesome. I was looking for something like that. Thank you for all the suggestions, really appreciate it!
…imeSource.BlockUntil in tests
fimanishi
force-pushed
the
tests-for-dlq_handler
branch
from
9e1109e
to
f04d44a
Compare
* Add tests for dlq_handler.go What changed? tests for dlq_handler.go. Moved getInterval function to dlqHandleImpl to allow testing Why? improve unit test coverage How did you test it? unit tests Potential risks Release notes Documentation Changes
What changed?
tests for dlq_handler.go. Moved getInterval function to dlqHandleImpl to allow testing
Why?
improve unit test coverage
How did you test it?
unit tests
Potential risks
getInterval behavior change due to change of location definition
Release notes
Documentation Changes