[go] os/signal: make TestTerminalSignal more reliable
25 views
Skip to first unread message
Ian Lance Taylor (Gerrit)
Nov 22, 2017, 10:14:47 AM11/22/17
to Michael Munday, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Ian Lance Taylor would like Michael Munday to review this change.
os/signal: make TestTerminalSignal more reliable
Look for program output and shell prompt to see when to continue.
Updates #22845
Change-Id: I44ed1908861f3b0dc098aee9a401324b77268921
---
M src/os/signal/signal_cgo_test.go
1 file changed, 69 insertions(+), 9 deletions(-)
diff --git a/src/os/signal/signal_cgo_test.go b/src/os/signal/signal_cgo_test.go
index 6eed979..653676f 100644
--- a/src/os/signal/signal_cgo_test.go
+++ b/src/os/signal/signal_cgo_test.go
@@ -9,6 +9,7 @@
import (
"bufio"
+ "bytes"
"context"
"fmt"
"io"
@@ -23,9 +24,10 @@
)
func TestTerminalSignal(t *testing.T) {
+ const enteringRead = "test program entering read"
if os.Getenv("GO_TEST_TERMINAL_SIGNALS") != "" {
var b [1]byte
- fmt.Println("entering read")
+ fmt.Println(enteringRead)
n, err := os.Stdin.Read(b[:])
fmt.Printf("read %d bytes: %q\n", n, b)
if err != nil {
@@ -35,6 +37,8 @@
os.Exit(0)
}
+ t.Parallel()
+
// The test requires a shell that uses job control.
bash, err := exec.LookPath("bash")
if err != nil {
@@ -47,6 +51,8 @@
scale = sc
}
}
+ pause := time.Duration(scale) * 10 * time.Millisecond
+ wait := time.Duration(scale) * 5 * time.Second
// The test only fails when using a "slow device," in this
// case a pseudo-terminal.
@@ -83,15 +89,20 @@
t.Errorf("closing slave: %v", err)
}
+ progReady := make(chan bool)
+ sawPrompt := make(chan bool, 10)
+ const prompt = "prompt> "
+
// Read data from master in the background.
go func() {
- buf := bufio.NewReader(master)
+ input := bufio.NewReader(master)
+ var line, handled []byte
for {
- data, err := buf.ReadBytes('\n')
- if len(data) > 0 {
- t.Logf("%q", data)
- }
+ b, err := input.ReadByte()
if err != nil {
+ if len(line) > 0 || len(handled) > 0 {
+ t.Logf("%q", append(handled, line...))
+ }
if perr, ok := err.(*os.PathError); ok {
err = perr.Err
}
@@ -103,37 +114,86 @@
}
return
}
+
+ if b == '\n' {
+ t.Logf("%q", append(handled, line...))
+ line = nil
+ handled = nil
+ continue
+ }
+
+ line = append(line, b)
+ if bytes.Contains(line, []byte(enteringRead)) {
+ close(progReady)
+ handled = append(handled, line...)
+ line = nil
+ } else if bytes.Contains(line, []byte(prompt)) && !bytes.Contains(line, []byte("PS1=")) {
+ sawPrompt <- true
+ handled = append(handled, line...)
+ line = nil
+ }
}
}()
+ // Set the bash prompt so that we can see it.
+ if _, err := master.Write([]byte("PS1='" + prompt + "'\n")); err != nil {
+ t.Fatal("setting prompt: %v", err)
+ }
+ select {
+ case <-sawPrompt:
+ case <-time.After(wait):
+ t.Fatal("timed out waiting for shell prompt")
+ }
+
// Start a small program that reads from stdin
// (namely the code at the top of this function).
if _, err := master.Write([]byte("GO_TEST_TERMINAL_SIGNALS=1 " + os.Args[0] + " -test.run=TestTerminalSignal\n")); err != nil {
t.Fatal(err)
}
+ // Wait for the program to print that it is starting.
+ select {
+ case <-progReady:
+ case <-time.After(wait):
+ t.Fatal("timed out waiting for program to start")
+ }
+
// Give the program time to enter the read call.
- time.Sleep(time.Duration(scale) * 100 * time.Millisecond)
+ time.Sleep(pause)
// Send a ^Z to stop the program.
if _, err := master.Write([]byte{26}); err != nil {
t.Fatalf("writing ^Z to pty: %v", err)
}
- // Give the process time to handle the signal.
- time.Sleep(time.Duration(scale) * 100 * time.Millisecond)
+ // Wait for the program to stop and return to the shell.
+ select {
+ case <-sawPrompt:
+ case <-time.After(wait):
+ t.Fatal("timed out waiting for shell prompt")
+ }
// Restart the stopped program.
if _, err := master.Write([]byte("fg\n")); err != nil {
t.Fatalf("writing %q to pty: %v", "fg", err)
}
+ // Give the process time to restart.
+ time.Sleep(pause)
+
// Write some data for the program to read,
// which should cause it to exit.
if _, err := master.Write([]byte{'\n'}); err != nil {
t.Fatalf("writing %q to pty: %v", "\n", err)
}
+ // Wait for the program to exit.
+ select {
+ case <-sawPrompt:
+ case <-time.After(wait):
+ t.Fatal("timed out waiting for shell prompt")
+ }
+
// Exit the shell with the program's exit status.
if _, err := master.Write([]byte("exit $?\n")); err != nil {
t.Fatalf("writing %q to pty: %v", "exit", err)
To view, visit change 79395. To unsubscribe, or for help writing mail filters, visit settings.
Gobot Gobot (Gerrit)
Nov 22, 2017, 10:15:00 AM11/22/17
to Ian Lance Taylor, goph...@pubsubhelper.golang.org, Michael Munday, golang-co...@googlegroups.com
TryBots beginning. Status page: https://farmer.golang.org/try?commit=5ecbdb77
Gobot Gobot (Gerrit)
Nov 22, 2017, 10:24:50 AM11/22/17
to Ian Lance Taylor, goph...@pubsubhelper.golang.org, Michael Munday, golang-co...@googlegroups.com
TryBots are happy.
Patch set 1:TryBot-Result +1
Michael Munday (Gerrit)
Nov 22, 2017, 11:03:41 AM11/22/17
to Ian Lance Taylor, goph...@pubsubhelper.golang.org, Gobot Gobot, golang-co...@googlegroups.com
This seems to make the test reliable on s390x at least.
Patch set 1:Code-Review +2
2 comments:
File src/os/signal/signal_cgo_test.go:
Maybe '"--norc", "--noprofile", "-i"' to get a clean terminal without aliases etc.? This might also mean you can check for "bash-" as the prompt instead of specifying your own.
We could check for 'GO_TEST_TERMINAL_SIGNALS=1' or similar to avoid the Sleep here. I think this might still technically be a race otherwise (based on the failures we've seen the '\n' seems to be discarded if sent too early). Maybe just a comment to that effect in case we see this being flaky later on? The other Sleep is fine.
To view, visit change 79395. To unsubscribe, or for help writing mail filters, visit settings.
Brad Fitzpatrick (Gerrit)
Nov 22, 2017, 11:40:14 AM11/22/17
to Ian Lance Taylor, goph...@pubsubhelper.golang.org, Brad Fitzpatrick, Michael Munday, Gobot Gobot, golang-co...@googlegroups.com
Ian Lance Taylor (Gerrit)
Nov 22, 2017, 1:39:38 PM11/22/17
to goph...@pubsubhelper.golang.org, Brad Fitzpatrick, Michael Munday, Gobot Gobot, golang-co...@googlegroups.com
2 comments:
Maybe '"--norc", "--noprofile", "-i"' to get a clean terminal without aliases etc. […]
Done
We could check for 'GO_TEST_TERMINAL_SIGNALS=1' or similar to avoid the Sleep here. […]
I added a comment and extended the sleep.
To view, visit change 79395. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Nov 22, 2017, 1:40:05 PM11/22/17
to Michael Munday, Gobot Gobot, Brad Fitzpatrick, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Ian Lance Taylor uploaded patch set #2 to this change.
os/signal: make TestTerminalSignal more reliable
Look for program output and shell prompt to see when to continue.
Updates #22845
Change-Id: I44ed1908861f3b0dc098aee9a401324b77268921
---
M src/os/signal/signal_cgo_test.go
1 file changed, 90 insertions(+), 11 deletions(-)
To view, visit change 79395. To unsubscribe, or for help writing mail filters, visit settings.
Gobot Gobot (Gerrit)
Nov 22, 2017, 1:40:18 PM11/22/17
to Ian Lance Taylor, goph...@pubsubhelper.golang.org, Brad Fitzpatrick, Michael Munday, golang-co...@googlegroups.com
Gobot Gobot (Gerrit)
Nov 22, 2017, 1:50:36 PM11/22/17
to Ian Lance Taylor, goph...@pubsubhelper.golang.org, Brad Fitzpatrick, Michael Munday, golang-co...@googlegroups.com
TryBots are happy.
Patch set 2:TryBot-Result +1
Ian Lance Taylor (Gerrit)
Nov 22, 2017, 2:19:07 PM11/22/17
to goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, Gobot Gobot, Brad Fitzpatrick, Michael Munday, golang-co...@googlegroups.com
Ian Lance Taylor merged this change.
Approvals:
Brad Fitzpatrick: Looks good to me, approved
Michael Munday: Looks good to me, approved
Ian Lance Taylor: Run TryBots
Gobot Gobot: TryBots succeeded
os/signal: make TestTerminalSignal more reliable
Look for program output and shell prompt to see when to continue.
Updates #22845
Change-Id: I44ed1908861f3b0dc098aee9a401324b77268921
Reviewed-on: https://go-review.googlesource.com/79395
Run-TryBot: Ian Lance Taylor <ia...@golang.org>
TryBot-Result: Gobot Gobot <go...@golang.org>
Reviewed-by: Michael Munday <mike....@ibm.com>
Reviewed-by: Brad Fitzpatrick <brad...@golang.org>
---
M src/os/signal/signal_cgo_test.go
1 file changed, 90 insertions(+), 11 deletions(-)
diff --git a/src/os/signal/signal_cgo_test.go b/src/os/signal/signal_cgo_test.go
index 6eed979..0daaacb 100644
--- a/src/os/signal/signal_cgo_test.go
+++ b/src/os/signal/signal_cgo_test.go
@@ -9,6 +9,7 @@
import (
"bufio"
+ "bytes"
"context"
"fmt"
"io"
@@ -23,11 +24,21 @@
)
func TestTerminalSignal(t *testing.T) {
+ const enteringRead = "test program entering read"
if os.Getenv("GO_TEST_TERMINAL_SIGNALS") != "" {
var b [1]byte
- fmt.Println("entering read")
+ fmt.Println(enteringRead)
n, err := os.Stdin.Read(b[:])
- fmt.Printf("read %d bytes: %q\n", n, b)
+ if n == 1 {
+ if b[0] == '\n' {
+ // This is what we expect
+ fmt.Println("read newline")
+ } else {
+ fmt.Printf("read 1 byte: %q\n", b)
+ }
+ } else {
+ fmt.Printf("read %d bytes\n", n)
+ }
if err != nil {
fmt.Println(err)
os.Exit(1)
@@ -35,6 +46,8 @@
os.Exit(0)
}
+ t.Parallel()
+
// The test requires a shell that uses job control.
bash, err := exec.LookPath("bash")
if err != nil {
@@ -47,6 +60,8 @@
scale = sc
}
}
+ pause := time.Duration(scale) * 10 * time.Millisecond
+ wait := time.Duration(scale) * 5 * time.Second
// The test only fails when using a "slow device," in this
// case a pseudo-terminal.
@@ -65,7 +80,7 @@
// Start an interactive shell.
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
defer cancel()
- cmd := exec.CommandContext(ctx, bash, "-i")
+ cmd := exec.CommandContext(ctx, bash, "--norc", "--noprofile", "-i")
cmd.Stdin = slave
cmd.Stdout = slave
cmd.Stderr = slave
@@ -83,15 +98,20 @@
t.Errorf("closing slave: %v", err)
}
+ progReady := make(chan bool)
+ sawPrompt := make(chan bool, 10)
+ const prompt = "prompt> "
+
// Read data from master in the background.
go func() {
- buf := bufio.NewReader(master)
+ input := bufio.NewReader(master)
+ var line, handled []byte
for {
- data, err := buf.ReadBytes('\n')
- if len(data) > 0 {
- t.Logf("%q", data)
- }
+ b, err := input.ReadByte()
if err != nil {
+ if len(line) > 0 || len(handled) > 0 {
+ t.Logf("%q", append(handled, line...))
+ }
if perr, ok := err.(*os.PathError); ok {
err = perr.Err
}
@@ -103,37 +123,96 @@
}
return
+ // It doesn't matter much if we occasionally don't wait long enough;
+ // we won't be testing what we want to test, but the overall test
+ // will pass.
+ time.Sleep(pause)
// Send a ^Z to stop the program.
if _, err := master.Write([]byte{26}); err != nil {
t.Fatalf("writing ^Z to pty: %v", err)
}
- // Give the process time to handle the signal.
- time.Sleep(time.Duration(scale) * 100 * time.Millisecond)
+ // Wait for the program to stop and return to the shell.
+ select {
+ case <-sawPrompt:
+ case <-time.After(wait):
+ t.Fatal("timed out waiting for shell prompt")
+ }
// Restart the stopped program.
if _, err := master.Write([]byte("fg\n")); err != nil {
t.Fatalf("writing %q to pty: %v", "fg", err)
}
+ // Give the process time to restart.
+ // This is potentially racy: if the process does not restart
+ // quickly enough then the byte we send will go to bash rather
+ // than the program. Unfortunately there isn't anything we can
+ // look for to know that the program is running again.
+ // bash will print the program name, but that happens before it
+ // restarts the program.
+ time.Sleep(10 * pause)
+
// Write some data for the program to read,
// which should cause it to exit.
if _, err := master.Write([]byte{'\n'}); err != nil {
t.Fatalf("writing %q to pty: %v", "\n", err)
}
+ // Wait for the program to exit.
+ select {
+ case <-sawPrompt:
+ case <-time.After(wait):
+ t.Fatal("timed out waiting for shell prompt")
+ }
+
// Exit the shell with the program's exit status.
if _, err := master.Write([]byte("exit $?\n")); err != nil {
t.Fatalf("writing %q to pty: %v", "exit", err)
To view, visit change 79395. To unsubscribe, or for help writing mail filters, visit settings.
Reply all
Reply to author
Forward
0 new messages