From 07c09f470d66f0ffa6689c3362c2e3f898e48a27 Mon Sep 17 00:00:00 2001 From: Josh Bleecher Snyder Date: Wed, 15 Sep 2021 16:40:54 -0700 Subject: [PATCH] ipn/ipnlocal: do not shut down the backend halfway through TestStateMachine LocalBackend.Shutdown's docs say: > The backend can no longer be used after Shutdown returns. Nevertheless, TestStateMachine blithely calls Shutdown, talks some smack, and continues on, expecting things to work. Other uses of Shutdown in the codebase are as intended. Things mostly kinda work anyway, except that the wgengine.Engine has been shut down, so calls to Reconfig fail. Those get logged: > local.go:603: wgengine status error: engine closing; no status but otherwise ignored. However, the Reconfig failure caused one fewer call to pause/unpause than normal. Now the assertCalls lines match the equivalent ones earlier in the test. I don't see an obvious correct replacement for Shutdown in the context of this test; I'm not sure entirely what it is trying to accomplish. It is possible that many of the tests remaining after the prior call to Shutdown are now extraneous. They don't harm anything, though, so err on the side of safety and leave them for now. Signed-off-by: Josh Bleecher Snyder --- ipn/ipnlocal/state_test.go | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/ipn/ipnlocal/state_test.go b/ipn/ipnlocal/state_test.go index bece9931b..a8261d302 100644 --- a/ipn/ipnlocal/state_test.go +++ b/ipn/ipnlocal/state_test.go @@ -645,16 +645,6 @@ func TestStateMachine(t *testing.T) { c.Assert(ipn.NeedsLogin, qt.Equals, b.State()) } - // Shut down the backend. - t.Logf("\n\nShutdown") - notifies.expect(0) - b.Shutdown() - { - notifies.drain(0) - // BUG: I expect a transition to ipn.NoState here. - cc.assertCalls("Shutdown") - } - // Oh, you thought we were done? Ha! Now we have to test what // happens if the user exits and restarts while logged out. // Note that it's explicitly okay to call b.Start() over and over @@ -694,7 +684,7 @@ func TestStateMachine(t *testing.T) { }) { nn := notifies.drain(3) - cc.assertCalls("unpause", "unpause") + cc.assertCalls("unpause", "unpause", "unpause") c.Assert(nn[0].LoginFinished, qt.IsNotNil) c.Assert(nn[1].Prefs, qt.IsNotNil) c.Assert(nn[2].State, qt.IsNotNil) @@ -771,7 +761,7 @@ func TestStateMachine(t *testing.T) { }) { nn := notifies.drain(2) - cc.assertCalls("Login", "unpause") + cc.assertCalls("Login", "unpause", "unpause") // BUG: I would expect Prefs to change first, and state after. c.Assert(nn[0].State, qt.IsNotNil) c.Assert(nn[1].Prefs, qt.IsNotNil) @@ -810,7 +800,7 @@ func TestStateMachine(t *testing.T) { // // Because the login hasn't yet completed, the old login // is still valid, so it's correct that we stay paused. - cc.assertCalls("Login", "pause") + cc.assertCalls("Login", "pause", "pause") c.Assert(nn[0].BrowseToURL, qt.IsNotNil) c.Assert(*nn[0].BrowseToURL, qt.Equals, url3) } @@ -832,7 +822,7 @@ func TestStateMachine(t *testing.T) { // and !WantRunning. But since it's a fresh and successful // new login, WantRunning is true, so there was never a // reason to pause(). - cc.assertCalls("pause", "unpause") + cc.assertCalls("pause", "unpause", "unpause") c.Assert(nn[0].LoginFinished, qt.IsNotNil) c.Assert(nn[1].Prefs, qt.IsNotNil) c.Assert(nn[2].State, qt.IsNotNil) @@ -870,7 +860,7 @@ func TestStateMachine(t *testing.T) { }) { nn := notifies.drain(1) - cc.assertCalls("unpause", "unpause") + cc.assertCalls("unpause", "unpause", "unpause") // NOTE: No LoginFinished message since no interactive // login was needed. c.Assert(nn[0].State, qt.IsNotNil)