Fully shutdown healthchecker on t.Cleanup()

[iain-macdonald]

Related issues: N/A

[bduffany]

I think as written, the shutdown functions will run asynchronously, while new tests are allowed to start executing, which doesn't seem ideal 🤔

I'd say we might want to call WaitForGracefulShutdown here but it looks like that has the side effect of starting the health checker loop, which might not be desired in tests? and it might increase test duration a bunch depending on how long the graceful shutdown takes

[tylerwilliams]

Does it make sense to have a testonly healthchecker that does something different here?

[bduffany]

hmm actually it looks like NewHealthChecker() is already starting the health checker loop. So the only concern would be increasing test duration. Maybe worth a try though and seeing whether tests get significantly slower or not. I would guess that graceful shutdown should be quick as long as we don't have any tests that are accidentally keeping RPC streams open at the end, preventing shutdown.

Speaking of testonly healthchecker though, one thing that we shouldn't do in tests is register the SIGINT / SIGTERM handlers - those are preventing Ctrl+C from working when running the test locally. But that's probably a separate issue.