Closed JacobOaks closed 1 month ago
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 98.51%. Comparing base (
abda254
) to head (1c5db75
).
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
lgtm with one minor behavioral improvement suggested
Thank you for all your helpful review @abhinav!
In Fx, if starting/stopping the application takes longer than the context's timeout, the fx.App will bail early, regardless of the status of the currently running hooks.
This prevents stalling an application when hooks (accidentally) block forever.
In order to test hook behavior, Fx provides fxtest.Lifecycle to interact with. This Lifecycle is a simple wrapper around the actual fx Lifecycle type, meaning it does not check for timeout and bail early like fx.App does. This is an issue because:
See #1180 for more details.
This PR adds an option that can be passed to
fxtest.NewLifecycle
to cause it to immediately fail when context expires, similar to fx.App.This PR doesn't provide a way to test timeouts using
RequireStart
andRequireStop
. However, since those don't take contexts anyways, my assumption is that usage of those APIs represents an intentional decision to not care about timeouts by the test writer.However, if people feel differently, we can instead do something like expose two new APIs
RequireStartWithContext(ctx)
andRequireStopWithContext(ctx)
or something (names obviously up for discussion).