Ensure scanner scavenger stops in tests #5510
Conversation
3vilhamster
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
Groxx,
shijiesheng,
agautam478,
jakobht,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
3vilhamster
force-pushed
the
ensure-scavenger-stop
branch
from
76f5ac3 to
6800ae1
Compare
3vilhamster
force-pushed
the
ensure-scavenger-stop
branch
from
6800ae1 to
4ba06fe
Compare
3vilhamster
force-pushed
the
ensure-scavenger-stop
branch
from
4ba06fe to
efd5d49
Compare
| defer s.scvgr.Stop() | ||
| timer := time.NewTimer(scavengerTestTimeout) | ||
| select { | ||
| case <-s.scvgr.stopped: |
There was a problem hiding this comment.
main caveat here is probably that, if it's in a live/dead-lock of some kind, it'll sit idle for N minutes until the whole test suite times out... though I guess it fails either way.
maybe that's better? does a timeout give you running goroutines? and do we have a reasonably-short go test-level timeout set, e.g. ideally 1 minute?
There was a problem hiding this comment.
looks like:
- yep, timeouts print stacks. should help find the issue when it occurs 👍
- outside the
host/ndcfolder (which has 20m), we have no specified timeout, which means currently "the whole package can test for 10? 30? minutes". and a small forever-blocking local test agrees with that, it's much longer than a minute at least.
so:
- 👍 yep I like this
- mind adding a default 1m timeout to all
go tests in the makefile?
There was a problem hiding this comment.
I'm not sure that we can broadly state that our test will timeout in 1m. I'd say we can try setting to 5m. I'll create a separate PR for this experiment.
There was a problem hiding this comment.
my manual test took 10m to finish, so that's apparently the default (and all the "the default is 30m" and "the default is 1m" that google finds are likely coming from discussion in golang/go#48157, and are not actually the defaults)
There was a problem hiding this comment.
broadly state a 1m timeout: yea. might not be good enough / a separate PR is fine IMO.
we can also selectively reduce the timeout (but I think not add?) with t.SetTimeout which might be good for these. long timeouts that eventually fail at an "obviously this could be caught MUCH earlier" are just really painful :\
What changed?
Ensure that scvngr is stopped if the test scenario times out.
Why?
If the test scenario will somehow take more than 10 seconds the test will fail, but can leak goroutine. So I ensure that the suite does the cleanup.
How did you test it?
Potential risks
Release notes
Documentation Changes