Skip to content

Commit 5622a4b

Browse files
prattmicprattmic
committed
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. Change-Id: Ibfce0f439508a7cfca848c7ccfd136c9c453ad8b Reviewed-on: https://go-review.googlesource.com/c/go/+/538899 LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Bryan Mills <[email protected]>
1 parent cfe36fd commit 5622a4b

1 file changed

Lines changed: 80 additions & 82 deletions

File tree

‎src/os/signal/signal_test.go‎

Lines changed: 80 additions & 82 deletions
Original file line numberDiff line numberDiff line change
@@ -408,12 +408,6 @@ func TestStop(t *testing.T) {
408408

409409
// Test that when run under nohup, an uncaught SIGHUP does not kill the program.
410410
func TestNohup(t *testing.T) {
411-
// Ugly: ask for SIGHUP so that child will not have no-hup set
412-
// even if test is running under nohup environment.
413-
// We have no intention of reading from c.
414-
c := make(chan os.Signal, 1)
415-
Notify(c, syscall.SIGHUP)
416-
417411
// When run without nohup, the test should crash on an uncaught SIGHUP.
418412
// When run under nohup, the test should ignore uncaught SIGHUPs,
419413
// because the runtime is not supposed to be listening for them.
@@ -425,88 +419,92 @@ func TestNohup(t *testing.T) {
425419
//
426420
// Both should fail without nohup and succeed with nohup.
427421

428-
var subTimeout time.Duration
429-
430-
var wg sync.WaitGroup
431-
wg.Add(2)
432-
if deadline, ok := t.Deadline(); ok {
433-
subTimeout = time.Until(deadline)
434-
subTimeout -= subTimeout / 10 // Leave 10% headroom for propagating output.
435-
}
436-
for i := 1; i <= 2; i++ {
437-
i := i
438-
go t.Run(fmt.Sprintf("uncaught-%d", i), func(t *testing.T) {
439-
defer wg.Done()
440-
441-
args := []string{
442-
"-test.v",
443-
"-test.run=^TestStop$",
444-
"-send_uncaught_sighup=" + strconv.Itoa(i),
445-
"-die_from_sighup",
446-
}
447-
if subTimeout != 0 {
448-
args = append(args, fmt.Sprintf("-test.timeout=%v", subTimeout))
449-
}
450-
out, err := testenv.Command(t, os.Args[0], args...).CombinedOutput()
451-
452-
if err == nil {
453-
t.Errorf("ran test with -send_uncaught_sighup=%d and it succeeded: expected failure.\nOutput:\n%s", i, out)
454-
} else {
455-
t.Logf("test with -send_uncaught_sighup=%d failed as expected.\nError: %v\nOutput:\n%s", i, err, out)
456-
}
457-
})
458-
}
459-
wg.Wait()
460-
461-
Stop(c)
422+
t.Run("uncaught", func(t *testing.T) {
423+
// Ugly: ask for SIGHUP so that child will not have no-hup set
424+
// even if test is running under nohup environment.
425+
// We have no intention of reading from c.
426+
c := make(chan os.Signal, 1)
427+
Notify(c, syscall.SIGHUP)
428+
t.Cleanup(func() { Stop(c) })
462429

463-
// Skip the nohup test below when running in tmux on darwin, since nohup
464-
// doesn't work correctly there. See issue #5135.
465-
if runtime.GOOS == "darwin" && os.Getenv("TMUX") != "" {
466-
t.Skip("Skipping nohup test due to running in tmux on darwin")
467-
}
430+
var subTimeout time.Duration
431+
if deadline, ok := t.Deadline(); ok {
432+
subTimeout = time.Until(deadline)
433+
subTimeout -= subTimeout / 10 // Leave 10% headroom for propagating output.
434+
}
435+
for i := 1; i <= 2; i++ {
436+
i := i
437+
t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
438+
t.Parallel()
439+
440+
args := []string{
441+
"-test.v",
442+
"-test.run=^TestStop$",
443+
"-send_uncaught_sighup=" + strconv.Itoa(i),
444+
"-die_from_sighup",
445+
}
446+
if subTimeout != 0 {
447+
args = append(args, fmt.Sprintf("-test.timeout=%v", subTimeout))
448+
}
449+
out, err := testenv.Command(t, os.Args[0], args...).CombinedOutput()
468450

469-
// Again, this time with nohup, assuming we can find it.
470-
_, err := exec.LookPath("nohup")
471-
if err != nil {
472-
t.Skip("cannot find nohup; skipping second half of test")
473-
}
451+
if err == nil {
452+
t.Errorf("ran test with -send_uncaught_sighup=%d and it succeeded: expected failure.\nOutput:\n%s", i, out)
453+
} else {
454+
t.Logf("test with -send_uncaught_sighup=%d failed as expected.\nError: %v\nOutput:\n%s", i, err, out)
455+
}
456+
})
457+
}
458+
})
474459

475-
wg.Add(2)
476-
if deadline, ok := t.Deadline(); ok {
477-
subTimeout = time.Until(deadline)
478-
subTimeout -= subTimeout / 10 // Leave 10% headroom for propagating output.
479-
}
480-
for i := 1; i <= 2; i++ {
481-
i := i
482-
go t.Run(fmt.Sprintf("nohup-%d", i), func(t *testing.T) {
483-
defer wg.Done()
460+
t.Run("nohup", func(t *testing.T) {
461+
// Skip the nohup test below when running in tmux on darwin, since nohup
462+
// doesn't work correctly there. See issue #5135.
463+
if runtime.GOOS == "darwin" && os.Getenv("TMUX") != "" {
464+
t.Skip("Skipping nohup test due to running in tmux on darwin")
465+
}
484466

485-
// POSIX specifies that nohup writes to a file named nohup.out if standard
486-
// output is a terminal. However, for an exec.Cmd, standard output is
487-
// not a terminal — so we don't need to read or remove that file (and,
488-
// indeed, cannot even create it if the current user is unable to write to
489-
// GOROOT/src, such as when GOROOT is installed and owned by root).
467+
// Again, this time with nohup, assuming we can find it.
468+
_, err := exec.LookPath("nohup")
469+
if err != nil {
470+
t.Skip("cannot find nohup; skipping second half of test")
471+
}
490472

491-
args := []string{
492-
os.Args[0],
493-
"-test.v",
494-
"-test.run=^TestStop$",
495-
"-send_uncaught_sighup=" + strconv.Itoa(i),
496-
}
497-
if subTimeout != 0 {
498-
args = append(args, fmt.Sprintf("-test.timeout=%v", subTimeout))
499-
}
500-
out, err := testenv.Command(t, "nohup", args...).CombinedOutput()
473+
var subTimeout time.Duration
474+
if deadline, ok := t.Deadline(); ok {
475+
subTimeout = time.Until(deadline)
476+
subTimeout -= subTimeout / 10 // Leave 10% headroom for propagating output.
477+
}
478+
for i := 1; i <= 2; i++ {
479+
i := i
480+
t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
481+
t.Parallel()
482+
483+
// POSIX specifies that nohup writes to a file named nohup.out if standard
484+
// output is a terminal. However, for an exec.Cmd, standard output is
485+
// not a terminal — so we don't need to read or remove that file (and,
486+
// indeed, cannot even create it if the current user is unable to write to
487+
// GOROOT/src, such as when GOROOT is installed and owned by root).
488+
489+
args := []string{
490+
os.Args[0],
491+
"-test.v",
492+
"-test.run=^TestStop$",
493+
"-send_uncaught_sighup=" + strconv.Itoa(i),
494+
}
495+
if subTimeout != 0 {
496+
args = append(args, fmt.Sprintf("-test.timeout=%v", subTimeout))
497+
}
498+
out, err := testenv.Command(t, "nohup", args...).CombinedOutput()
501499

502-
if err != nil {
503-
t.Errorf("ran test with -send_uncaught_sighup=%d under nohup and it failed: expected success.\nError: %v\nOutput:\n%s", i, err, out)
504-
} else {
505-
t.Logf("ran test with -send_uncaught_sighup=%d under nohup.\nOutput:\n%s", i, out)
506-
}
507-
})
508-
}
509-
wg.Wait()
500+
if err != nil {
501+
t.Errorf("ran test with -send_uncaught_sighup=%d under nohup and it failed: expected success.\nError: %v\nOutput:\n%s", i, err, out)
502+
} else {
503+
t.Logf("ran test with -send_uncaught_sighup=%d under nohup.\nOutput:\n%s", i, out)
504+
}
505+
})
506+
}
507+
})
510508
}
511509

512510
// Test that SIGCONT works (issue 8953).

0 commit comments

Comments
 (0)