From 1d9def43146e8a2cbe729c513b4b6d66901cff71 Mon Sep 17 00:00:00 2001 From: zhangwenjian Date: Mon, 7 Sep 2026 15:14:39 +0800 Subject: [PATCH] =?UTF-8?q?test=E2=9C=85:=20restore=20the=20mode=20before?= =?UTF-8?q?=20the=20route=20helper=20returns?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit registeredRoutes set config.ApplicationConfig.Mode and gave it back with t.Cleanup, which runs at the end of the test rather than at the end of the helper. Everything the caller did after the call therefore ran under the mode the helper had been asked about, not one the caller chose. Nothing was wrong yet: the one caller that reads the mode afterwards sets it itself, and the two cleanups happen to unwind in an order that leaves the right value. Both of those are accidents, and neither is visible at the call site. A defer inside the helper makes the borrowing end where it starts. The doc comment said the mode was put back before returning while the code did not, so that is now true rather than aspirational. The counter-proof is the reason this has a test of its own: with t.Cleanup back in place TestRegisteredRoutesRestoresTheModeBeforeReturning fails and nothing else does, which is what a leak this quiet looks like when something is actually watching for it. Raised by Copilot on #917. --- app/other/router/demo_guard_test.go | 11 ++++++++--- app/other/router/gen_router_test.go | 28 +++++++++++++++++++++++++--- 2 files changed, 33 insertions(+), 6 deletions(-) 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) + } +}