tstest/natlab/vmtest: add helpers for fatal step errors (#19753)

In a lot of places, we construct an error to End a step, then immediately log
it to the governing test as test fatal. Save ourselves a bit of boilerplate by
putting methods on Step for that.

There are a couple cases this doesn't cover, e.g., where we construct the Step
outside a subtest that wants to fail individually, but it helps enough to pay
for its lines.

Updates #13038

Change-Id: I71f9900942962de16609b6b198d3ba13d6958a5f
Signed-off-by: M. J. Fromberger <fromberger@tailscale.com>

tstest/natlab/vmtest/vmstatus.go

@@ -129,6 +129,24 @@ func (s *Step) End(err error) {
 	s.env.publishStepChange(s)
 }
 
+// Fatalf marks the step as failed (as [Step.End]), and then logs a test
+// failure to the environment's test, with an error constructed from the given
+// arguments.
+func (s *Step) Fatalf(msg string, args ...any) {
+	s.Fatal(fmt.Errorf(msg, args...))
+}
+
+// Fatal marks the step as failed (as [Step.End]), and then logs a test failure
+// to the environment's test, with the specified (non-nil) error. It will panic
+// if err == nil.
+func (s *Step) Fatal(err error) {
+	if err == nil {
+		panic(fmt.Sprintf("Step %q: Fatal called with a nil error", s.name))
+	}
+	s.End(err)
+	s.env.t.Fatal(err)
+}
+
 // EventType identifies the kind of event published to the EventBus.
 type EventType string
 

tstest/natlab/vmtest/vmtest_test.go

@@ -110,8 +110,7 @@ func testSubnetRouterForOS(t testing.TB, srOS vmtest.OSImage) {
 	httpStep.Begin()
 	body := env.HTTPGet(client, fmt.Sprintf("http://%s:8080/", backend.LanIP(internalNet)))
 	if !strings.Contains(body, "Hello world I am backend") {
-		httpStep.End(fmt.Errorf("got %q", body))
+		httpStep.Fatalf("got %q", body)
-		t.Fatalf("got %q", body)
 	}
 	httpStep.End(nil)
 }
@@ -198,16 +197,14 @@ func testSiteToSite(t *testing.T, srOS vmtest.OSImage) {
 	t.Logf("response: %s", body)
 
 	if !strings.Contains(body, "Hello world I am backend-b") {
-		httpStep.End(fmt.Errorf("expected response from backend-b, got %q", body))
+		httpStep.Fatalf("expected response from backend-b, got %q", body)
-		t.Fatalf("expected response from backend-b, got %q", body)
 	}
 
 	// Verify the source IP was preserved. With --snat-subnet-routes=false,
 	// backend-b should see backend-a's LAN IP as the source, not sr-b's LAN IP.
 	backendAIP := backendA.LanIP(lanA).String()
 	if !strings.Contains(body, "from "+backendAIP) {
-		httpStep.End(fmt.Errorf("source IP not preserved: expected %q in response, got %q", backendAIP, body))
+		httpStep.Fatalf("source IP not preserved: expected %q in response, got %q", backendAIP, body)
-		t.Fatalf("source IP not preserved: expected %q in response, got %q", backendAIP, body)
 	}
 	httpStep.End(nil)
 }
@@ -244,12 +241,10 @@ func TestInterNetworkTCP(t *testing.T) {
 	body := env.HTTPGet(client, fmt.Sprintf("http://%s:8080/", webWAN))
 	t.Logf("response: %s", body)
 	if !strings.Contains(body, "Hello world I am webserver") {
-		httpStep.End(fmt.Errorf("unexpected response: %q", body))
+		httpStep.Fatalf("unexpected response: %q", body)
-		t.Fatalf("unexpected response: %q", body)
 	}
 	if !strings.Contains(body, "from "+clientWAN) {
-		httpStep.End(fmt.Errorf("expected source %q in response, got %q", clientWAN, body))
+		httpStep.Fatalf("expected source %q in response, got %q", clientWAN, body)
-		t.Fatalf("expected source %q in response, got %q", clientWAN, body)
 	}
 	httpStep.End(nil)
 }
@@ -308,12 +303,10 @@ func TestSubnetRouterPublicIP(t *testing.T) {
 		body := env.HTTPGet(client, webURL)
 		t.Logf("[%s] response: %s", label, body)
 		if !strings.Contains(body, "Hello world I am webserver") {
-			step.End(fmt.Errorf("[%s] unexpected webserver response: %q", label, body))
+			step.Fatalf("[%s] unexpected webserver response: %q", label, body)
-			t.Fatalf("[%s] unexpected webserver response: %q", label, body)
 		}
 		if !strings.Contains(body, "from "+wantSrc) {
-			step.End(fmt.Errorf("[%s] expected source %q in response, got %q", label, wantSrc, body))
+			step.Fatalf("[%s] expected source %q in response, got %q", label, wantSrc, body)
-			t.Fatalf("[%s] expected source %q in response, got %q", label, wantSrc, body)
 		}
 		step.End(nil)
 	}
@@ -458,15 +451,11 @@ func TestTaildrop(t *testing.T) {
 
 	verifyStep.Begin()
 	if gotName != filename {
-		err := fmt.Errorf("received name = %q; want %q", gotName, filename)
+		verifyStep.Fatalf("received name = %q; want %q", gotName, filename)
-		verifyStep.End(err)
-		t.Error(err)
 		return
 	}
 	if !bytes.Equal(gotContent, want) {
-		err := fmt.Errorf("received content = %q; want %q", gotContent, want)
+		verifyStep.Fatalf("received content = %q; want %q", gotContent, want)
-		verifyStep.End(err)
-		t.Error(err)
 		return
 	}
 	verifyStep.End(nil)
@@ -542,12 +531,10 @@ func TestExitNode(t *testing.T) {
 			body := env.HTTPGet(client, webURL)
 			t.Logf("response: %s", body)
 			if !strings.Contains(body, "Hello world I am webserver") {
-				tt.step.End(fmt.Errorf("unexpected webserver response: %q", body))
+				tt.step.Fatalf("unexpected webserver response: %q", body)
-				t.Fatalf("unexpected webserver response: %q", body)
 			}
 			if !strings.Contains(body, "from "+tt.wantSrc) {
-				tt.step.End(fmt.Errorf("expected source %q in response, got %q", tt.wantSrc, body))
+				tt.step.Fatalf("expected source %q in response, got %q", tt.wantSrc, body)
-				t.Fatalf("expected source %q in response, got %q", tt.wantSrc, body)
 			}
 			tt.step.End(nil)
 		})
@@ -674,8 +661,7 @@ func TestDiscoKeyChange(t *testing.T) {
 
 	pingABStep.Begin()
 	if err := env.Ping(a, b, tailcfg.PingTSMP, 30*time.Second); err != nil {
-		pingABStep.End(err)
+		pingABStep.Fatal(err)
-		t.Fatal(err)
 	}
 	pingABStep.End(nil)
 
@@ -719,9 +705,7 @@ func checkDiscoRotated(t *testing.T, env *vmtest.Env, a, b, pingFrom, pingTo *vm
 	verifyStep.Begin()
 	bSt := env.Status(b)
 	if got := bSt.Self.PublicKey; got != bNodeKey {
-		err := fmt.Errorf("[%s] b's node key changed: %v -> %v", label, bNodeKey, got)
+		verifyStep.Fatalf("[%s] b's node key changed: %v -> %v", label, bNodeKey, got)
-		verifyStep.End(err)
-		t.Fatal(err)
 	}
 	var newDisco key.DiscoPublic
 	if err := tstest.WaitFor(15*time.Second, func() error {
@@ -735,8 +719,7 @@ func checkDiscoRotated(t *testing.T, env *vmtest.Env, a, b, pingFrom, pingTo *vm
 		newDisco = n.DiscoKey
 		return nil
 	}); err != nil {
-		verifyStep.End(err)
+		verifyStep.Fatalf("[%s] %v", label, err)
-		t.Fatalf("[%s] %v", label, err)
 	}
 	t.Logf("[b] after %s: nodekey=%s discokey=%s", label, bNodeKey.ShortString(), newDisco.ShortString())
 	verifyStep.End(nil)
@@ -897,12 +880,10 @@ func TestMullvadExitNode(t *testing.T) {
 		body := env.HTTPGet(client, webURL)
 		t.Logf("[%s] response: %s", label, body)
 		if !strings.Contains(body, "Hello world I am webserver") {
-			step.End(fmt.Errorf("[%s] unexpected webserver response: %q", label, body))
+			step.Fatalf("[%s] unexpected webserver response: %q", label, body)
-			t.Fatalf("[%s] unexpected webserver response: %q", label, body)
 		}
 		if !strings.Contains(body, "from "+wantSrc) {
-			step.End(fmt.Errorf("[%s] expected source %q in response, got %q", label, wantSrc, body))
+			step.Fatalf("[%s] expected source %q in response, got %q", label, wantSrc, body)
-			t.Fatalf("[%s] expected source %q in response, got %q", label, wantSrc, body)
 		}
 		step.End(nil)
 	}
@@ -965,8 +946,7 @@ func TestCachedNetmapAfterRestart(t *testing.T) {
 
 	connectStep.Begin()
 	if err := env.Ping(a, b, tailcfg.PingTSMP, 30*time.Second); err != nil {
-		connectStep.End(err)
+		connectStep.Fatal(err)
-		t.Fatal(err)
 	}
 	connectStep.End(nil)
 
@@ -988,12 +968,10 @@ func TestCachedNetmapAfterRestart(t *testing.T) {
 	for _, node := range []*vmtest.Node{a, b} {
 		nm, err := local.GetDebugResultJSON[netmap.NetworkMap](t.Context(), node.Agent().Client, "current-netmap")
 		if err != nil {
-			netmapCheckStep.End(fmt.Errorf("[%s] got err fetching netmap %q", node.Name(), err))
+			netmapCheckStep.Fatalf("[%s] got err fetching netmap %q", node.Name(), err)
-			t.Fatalf("[%s] got err fetching netmap %q", node.Name(), err)
 		}
 		if !nm.Cached {
-			netmapCheckStep.End(fmt.Errorf("[%s] expected netmap.Cached = true, got: %t", node.Name(), nm.Cached))
+			netmapCheckStep.Fatalf("[%s] expected netmap.Cached = true, got: %t", node.Name(), nm.Cached)
-			t.Fatalf("[%s] expected netmap.Cached = true, got: %t", node.Name(), nm.Cached)
 		}
 	}
 	netmapCheckStep.End(nil)
@@ -1010,8 +988,7 @@ func TestCachedNetmapAfterRestart(t *testing.T) {
 	// because every timer fires multiple times under scheduling jitter.
 	pingStep.Begin()
 	if err := env.Ping(a, b, tailcfg.PingTSMP, 90*time.Second); err != nil {
-		pingStep.End(err)
+		pingStep.Fatal(err)
-		t.Fatal(err)
 	}
 	pingStep.End(nil)
 }
@@ -1066,15 +1043,13 @@ func TestDirectConnectionWithCachedNetmapOnOneNode(t *testing.T) {
 
 	tsmpPingStep.Begin()
 	if err := env.Ping(a, b, tailcfg.PingTSMP, 30*time.Second); err != nil {
-		tsmpPingStep.End(err)
+		tsmpPingStep.Fatal(err)
-		t.Fatal(err)
 	}
 	tsmpPingStep.End(nil)
 
 	discoPingStep.Begin()
 	if err := env.PingExpect(a, b, vmtest.PingRouteDirect, 30*time.Second); err != nil {
-		discoPingStep.End(err)
+		discoPingStep.Fatal(err)
-		t.Fatal(err)
 	}
 	discoPingStep.End(nil)