[release-branch.go1.20] os/signal: remove go t.Run from TestNohup

Since CL 226138, TestNohup has a bit of a strange construction: it wants to run the "uncaught" subtests in parallel with each other, and the "nohup" subtests in parallel with each other, but also needs join between "uncaught" and "nohop" so it can Stop notifying for SIGHUP. It achieves this by doing `go t.Run` with a WaitGroup rather than using `t.Parallel` in the subtest (which would make `t.Run` return immediately). However, this makes things more difficult to understand than necessary. As noted on https://pkg.go.dev/testing#hdr-Subtests_and_Sub_benchmarks, a second layer of subtest can be used to join parallel subtests. Switch to this form, which makes the test simpler to follow (particularly the cleanup that goes with "uncaught"). For #63799. For #63910. Change-Id: Ibfce0f439508a7cfca848c7ccfd136c9c453ad8b Reviewed-on: https://go-review.googlesource.com/c/go/+/538899 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Bryan Mills <bcmills@google.com> (cherry picked from commit 5622a4b) Reviewed-on: https://go-review.googlesource.com/c/go/+/546375
golang · Dec 7, 2023 · 59ffd3b · 59ffd3b
1 parent 97c8ff8
commit 59ffd3b
Showing 1 changed file with 80 additions and 82 deletions.
diff --git a/src/os/signal/signal_test.go b/src/os/signal/signal_test.go
@@ -408,12 +408,6 @@ func TestStop(t *testing.T) {
 
 // Test that when run under nohup, an uncaught SIGHUP does not kill the program.
 func TestNohup(t *testing.T) {
-	// Ugly: ask for SIGHUP so that child will not have no-hup set
-	// even if test is running under nohup environment.
-	// We have no intention of reading from c.
-	c := make(chan os.Signal, 1)
-	Notify(c, syscall.SIGHUP)
-
 	// When run without nohup, the test should crash on an uncaught SIGHUP.
 	// When run under nohup, the test should ignore uncaught SIGHUPs,
 	// because the runtime is not supposed to be listening for them.
@@ -425,88 +419,92 @@ func TestNohup(t *testing.T) {
 	//
 	// Both should fail without nohup and succeed with nohup.
 
-	var subTimeout time.Duration
-
-	var wg sync.WaitGroup
-	wg.Add(2)
-	if deadline, ok := t.Deadline(); ok {
-		subTimeout = time.Until(deadline)
-		subTimeout -= subTimeout / 10 // Leave 10% headroom for propagating output.
-	}
-	for i := 1; i <= 2; i++ {
-		i := i
-		go t.Run(fmt.Sprintf("uncaught-%d", i), func(t *testing.T) {
-			defer wg.Done()
-
-			args := []string{
-				"-test.v",
-				"-test.run=TestStop",
-				"-send_uncaught_sighup=" + strconv.Itoa(i),
-				"-die_from_sighup",
-			}
-			if subTimeout != 0 {
-				args = append(args, fmt.Sprintf("-test.timeout=%v", subTimeout))
-			}
-			out, err := exec.Command(os.Args[0], args...).CombinedOutput()
-
-			if err == nil {
-				t.Errorf("ran test with -send_uncaught_sighup=%d and it succeeded: expected failure.\nOutput:\n%s", i, out)
-			} else {
-				t.Logf("test with -send_uncaught_sighup=%d failed as expected.\nError: %v\nOutput:\n%s", i, err, out)
-			}
-		})
-	}
-	wg.Wait()
-
-	Stop(c)
+	t.Run("uncaught", func(t *testing.T) {
+		// Ugly: ask for SIGHUP so that child will not have no-hup set
+		// even if test is running under nohup environment.
+		// We have no intention of reading from c.
+		c := make(chan os.Signal, 1)
+		Notify(c, syscall.SIGHUP)
+		t.Cleanup(func() { Stop(c) })
 
-	// Skip the nohup test below when running in tmux on darwin, since nohup
-	// doesn't work correctly there. See issue #5135.
-	if runtime.GOOS == "darwin" && os.Getenv("TMUX") != "" {
-		t.Skip("Skipping nohup test due to running in tmux on darwin")
-	}
+		var subTimeout time.Duration
+		if deadline, ok := t.Deadline(); ok {
+			subTimeout = time.Until(deadline)
+			subTimeout -= subTimeout / 10 // Leave 10% headroom for propagating output.
+		}
+		for i := 1; i <= 2; i++ {
+			i := i
+			t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
+				t.Parallel()
+
+				args := []string{
+					"-test.v",
+					"-test.run=TestStop",
+					"-send_uncaught_sighup=" + strconv.Itoa(i),
+					"-die_from_sighup",
+				}
+				if subTimeout != 0 {
+					args = append(args, fmt.Sprintf("-test.timeout=%v", subTimeout))
+				}
+				out, err := testenv.Command(t, os.Args[0], args...).CombinedOutput()
 
-	// Again, this time with nohup, assuming we can find it.
-	_, err := exec.LookPath("nohup")
-	if err != nil {
-		t.Skip("cannot find nohup; skipping second half of test")
-	}
+				if err == nil {
+					t.Errorf("ran test with -send_uncaught_sighup=%d and it succeeded: expected failure.\nOutput:\n%s", i, out)
+				} else {
+					t.Logf("test with -send_uncaught_sighup=%d failed as expected.\nError: %v\nOutput:\n%s", i, err, out)
+				}
+			})
+		}
+	})
 
-	wg.Add(2)
-	if deadline, ok := t.Deadline(); ok {
-		subTimeout = time.Until(deadline)
-		subTimeout -= subTimeout / 10 // Leave 10% headroom for propagating output.
-	}
-	for i := 1; i <= 2; i++ {
-		i := i
-		go t.Run(fmt.Sprintf("nohup-%d", i), func(t *testing.T) {
-			defer wg.Done()
+	t.Run("nohup", func(t *testing.T) {
+		// Skip the nohup test below when running in tmux on darwin, since nohup
+		// doesn't work correctly there. See issue #5135.
+		if runtime.GOOS == "darwin" && os.Getenv("TMUX") != "" {
+			t.Skip("Skipping nohup test due to running in tmux on darwin")
+		}
 
-			// POSIX specifies that nohup writes to a file named nohup.out if standard
-			// output is a terminal. However, for an exec.Command, standard output is
-			// not a terminal — so we don't need to read or remove that file (and,
-			// indeed, cannot even create it if the current user is unable to write to
-			// GOROOT/src, such as when GOROOT is installed and owned by root).
+		// Again, this time with nohup, assuming we can find it.
+		_, err := exec.LookPath("nohup")
+		if err != nil {
+			t.Skip("cannot find nohup; skipping second half of test")
+		}
 
-			args := []string{
-				os.Args[0],
-				"-test.v",
-				"-test.run=TestStop",
-				"-send_uncaught_sighup=" + strconv.Itoa(i),
-			}
-			if subTimeout != 0 {
-				args = append(args, fmt.Sprintf("-test.timeout=%v", subTimeout))
-			}
-			out, err := exec.Command("nohup", args...).CombinedOutput()
+		var subTimeout time.Duration
+		if deadline, ok := t.Deadline(); ok {
+			subTimeout = time.Until(deadline)
+			subTimeout -= subTimeout / 10 // Leave 10% headroom for propagating output.
+		}
+		for i := 1; i <= 2; i++ {
+			i := i
+			t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
+				t.Parallel()
+
+				// POSIX specifies that nohup writes to a file named nohup.out if standard
+				// output is a terminal. However, for an exec.Cmd, standard output is
+				// not a terminal — so we don't need to read or remove that file (and,
+				// indeed, cannot even create it if the current user is unable to write to
+				// GOROOT/src, such as when GOROOT is installed and owned by root).
+
+				args := []string{
+					os.Args[0],
+					"-test.v",
+					"-test.run=TestStop",
+					"-send_uncaught_sighup=" + strconv.Itoa(i),
+				}
+				if subTimeout != 0 {
+					args = append(args, fmt.Sprintf("-test.timeout=%v", subTimeout))
+				}
+				out, err := testenv.Command(t, "nohup", args...).CombinedOutput()
 
-			if err != nil {
-				t.Errorf("ran test with -send_uncaught_sighup=%d under nohup and it failed: expected success.\nError: %v\nOutput:\n%s", i, err, out)
-			} else {
-				t.Logf("ran test with -send_uncaught_sighup=%d under nohup.\nOutput:\n%s", i, out)
-			}
-		})
-	}
-	wg.Wait()
+				if err != nil {
+					t.Errorf("ran test with -send_uncaught_sighup=%d under nohup and it failed: expected success.\nError: %v\nOutput:\n%s", i, err, out)
+				} else {
+					t.Logf("ran test with -send_uncaught_sighup=%d under nohup.\nOutput:\n%s", i, out)
+				}
+			})
+		}
+	})
 }
 
 // Test that SIGCONT works (issue 8953).