Skip to content

Commit

Permalink
fix flaky TestFilesAreNonblocking on CI
Browse files Browse the repository at this point in the history
CI runs test with stdin closed, which leads our victim cat process
to exit quickly. Depending on the ordering of events we might see
the test fail. Change the contract for newOSProcess to include
stdin, stdout and stderr and allocate a blocking pipe as stdin for
the test.
  • Loading branch information
lmb committed Jan 25, 2021
1 parent 9fd66fb commit 293fa4b
Show file tree
Hide file tree
Showing 4 changed files with 26 additions and 17 deletions.
2 changes: 1 addition & 1 deletion child.go
Expand Up @@ -31,7 +31,7 @@ func startChild(env *env, passedFiles map[fileName]*file) (*child, error) {
}

// Copy passed fds and append the notification pipe
fds := []*os.File{readyW, namesR}
fds := []*os.File{os.Stdin, os.Stdout, os.Stderr, readyW, namesR}
var fdNames [][]string
for name, file := range passedFiles {
nameSlice := make([]string, len(name))
Expand Down
12 changes: 6 additions & 6 deletions child_test.go
Expand Up @@ -83,24 +83,24 @@ func TestChildPassedFds(t *testing.T) {
}

in := map[fileName]*file{
fileName{"r"}: newFile(r.Fd(), fileName{"r"}),
fileName{"w"}: newFile(w.Fd(), fileName{"w"}),
{"r"}: newFile(r.Fd(), fileName{"r"}),
{"w"}: newFile(w.Fd(), fileName{"w"}),
}

if _, err := startChild(env, in); err != nil {
t.Fatal(err)
}

proc := <-procs
if len(proc.fds) != 2+2 {
t.Error("Expected 4 files, got", len(proc.fds))
}

out, _, err := proc.notify()
if err != nil {
t.Fatal("Notify failed:", err)
}

if len(out) != len(in) {
t.Errorf("Expected %d files, got %d", len(in), len(out))
}

for name, inFd := range in {
if outFd, ok := out[name]; !ok {
t.Error(name, "is missing")
Expand Down
3 changes: 1 addition & 2 deletions process.go
Expand Up @@ -21,13 +21,12 @@ type osProcess struct {
finished bool
}

func newOSProcess(executable string, args []string, extraFiles []*os.File, env []string) (process, error) {
func newOSProcess(executable string, args []string, files []*os.File, env []string) (process, error) {
executable, err := exec.LookPath(executable)
if err != nil {
return nil, err
}

files := append([]*os.File{os.Stdin, os.Stdout, os.Stderr}, extraFiles...)
fds := make([]uintptr, 0, len(files))
for _, file := range files {
fd, err := sysConnFd(file)
Expand Down
26 changes: 18 additions & 8 deletions process_test.go
Expand Up @@ -12,18 +12,28 @@ import (
)

func TestFilesAreNonblocking(t *testing.T) {
r, w, err := os.Pipe()
if err != nil {
t.Fatal(err)
pipe := func() (r, w *os.File) {
r, w, err := os.Pipe()
if err != nil {
t.Fatal(err)
}
t.Cleanup(func() {
r.Close()
w.Close()
})
return r, w
}
defer r.Close()
defer w.Close()

// Set up our own blocking stdin since CI runs tests with stdin closed.
rStdin, _ := pipe()
rStdin.Fd()

r, _ := pipe()
if !isNonblock(t, r) {
t.Fatal("Read pipe is blocking")
}

proc, err := newOSProcess("cat", nil, []*os.File{r}, nil)
proc, err := newOSProcess("cat", nil, []*os.File{rStdin, os.Stdout, os.Stderr, r}, nil)
if err != nil {
t.Fatal(err)
}
Expand All @@ -47,7 +57,7 @@ func TestFilesAreNonblocking(t *testing.T) {
}

func TestArgumentsArePassedCorrectly(t *testing.T) {
proc, err := newOSProcess("printf", []string{""}, nil, nil)
proc, err := newOSProcess("printf", []string{""}, []*os.File{os.Stdin, os.Stdout, os.Stderr}, nil)
if err != nil {
t.Fatal("Can't execute printf:", err)
}
Expand Down Expand Up @@ -103,7 +113,7 @@ func newTestProcess(fds []*os.File, envstr []string) (*testProcess, error) {
fds,
env{
newFile: func(fd uintptr, name string) *os.File {
return fds[fd-3]
return fds[fd]
},
getenv: func(key string) string {
return environ[key]
Expand Down

0 comments on commit 293fa4b

Please sign in to comment.