diff --git a/app/other/router/demo_guard_test.go b/app/other/router/demo_guard_test.go index 15a7d198..184b817d 100644 --- a/app/other/router/demo_guard_test.go +++ b/app/other/router/demo_guard_test.go @@ -16,8 +16,13 @@ import ( // The mode has to be given rather than inherited, because it now decides what // gets registered: a test that leaves it at the zero value would be asking // about a mode no deployment runs in, and would pass whether or not the gate -// works. The previous value is put back so the order of tests in this package -// cannot change their answers. +// works. +// +// It is put back before this returns, not at the end of the test. t.Cleanup +// would leave the mode set for everything the caller does afterwards, so a +// caller that went on to assert something mode-dependent would be reading a +// value this helper left behind rather than one it chose. A caller that does +// want the mode set has to set it, which is visible where it happens. // // The JWT middleware is a zero value. MiddlewareFunc only closes over the // receiver and is never called here - no request is served, the engine is @@ -27,7 +32,7 @@ func registeredRoutes(t *testing.T, mode string) map[string]bool { gin.SetMode(gin.TestMode) previous := config.ApplicationConfig.Mode - t.Cleanup(func() { config.ApplicationConfig.Mode = previous }) + defer func() { config.ApplicationConfig.Mode = previous }() config.ApplicationConfig.Mode = mode r := gin.New() diff --git a/app/other/router/gen_router_test.go b/app/other/router/gen_router_test.go index 01452766..5f102dfe 100644 --- a/app/other/router/gen_router_test.go +++ b/app/other/router/gen_router_test.go @@ -82,9 +82,9 @@ func TestGenWriteRoutesEnabledAgreesWithWhatWasRegistered(t *testing.T) { t.Run("mode="+mode, func(t *testing.T) { routes := registeredRoutes(t, mode) - // registeredRoutes puts the mode back before it returns, so it - // has to be set again to ask the predicate about the same mode - // the engine was built under. + // registeredRoutes puts the mode back before it returns, so ask + // the predicate under a mode set here - about the same value the + // engine was just built under. previous := config.ApplicationConfig.Mode t.Cleanup(func() { config.ApplicationConfig.Mode = previous }) config.ApplicationConfig.Mode = mode @@ -98,3 +98,25 @@ func TestGenWriteRoutesEnabledAgreesWithWhatWasRegistered(t *testing.T) { }) } } + +// The helper restores the mode before it returns, so nothing it was asked +// about leaks into what the caller does next. +// +// Worth a test of its own because the failure is silent: a helper that left +// the mode set would make every assertion after the call read a value the +// caller did not choose, and each of those assertions would still pass for as +// long as the leaked value happened to be the right one. +func TestRegisteredRoutesRestoresTheModeBeforeReturning(t *testing.T) { + const sentinel = "not-a-mode" + + previous := config.ApplicationConfig.Mode + t.Cleanup(func() { config.ApplicationConfig.Mode = previous }) + config.ApplicationConfig.Mode = sentinel + + registeredRoutes(t, "prod") + + if got := config.ApplicationConfig.Mode; got != sentinel { + t.Errorf("mode after the helper returned = %q, want %q - it was left set to what "+ + "the helper was asked about", got, sentinel) + } +}