Skip to content

Commit 06a498a

Browse files
adonovangopherbot
adonovan
authored and
gopherbot
committed
go/ssa/interp: simplify output capturing
This CL uses the new output-capturing mechanism (via a pipe) exclusively, eliminating two old mechanisms: 1) the CapturedOutput global var that was written by print and println; now those two functions just write to stderr, which is more faithful (than stdout). 2) TestRangeFunc called the interpreter in a subprocess so that it could capture output (via pipes), but that is no longer necessary; 'run' returns the captured output. Also: - t.Log the duration unconditionally; remove "if false" cruft. - eliminate the unnecessary cwd arguments to filepath.Join. Change-Id: Ib23f895fafecab059ecda60f73fb7082388f0240 Reviewed-on: https://go-review.googlesource.com/c/tools/+/622416 Reviewed-by: Tim King <[email protected]> Auto-Submit: Alan Donovan <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
1 parent b2321e7 commit 06a498a

File tree

3 files changed

+63
-136
lines changed

3 files changed

+63
-136
lines changed

‎go/ssa/interp/interp_test.go

Lines changed: 14 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@ import (
2121
"go/build"
2222
"go/types"
2323
"io"
24-
"log"
2524
"os"
2625
"path/filepath"
2726
"runtime"
@@ -152,7 +151,8 @@ func init() {
152151
os.Setenv("GOARCH", runtime.GOARCH)
153152
}
154153

155-
func run(t *testing.T, input string, goroot string) {
154+
// run runs a single test. On success it returns the captured std{out,err}.
155+
func run(t *testing.T, input string, goroot string) string {
156156
testenv.NeedsExec(t) // really we just need os.Pipe, but os/exec uses pipes
157157

158158
t.Logf("Input: %s\n", input)
@@ -182,14 +182,13 @@ func run(t *testing.T, input string, goroot string) {
182182
// Print a helpful hint if we don't make it to the end.
183183
var hint string
184184
defer func() {
185+
t.Logf("Duration: %v", time.Since(start))
185186
if hint != "" {
186187
t.Log("FAIL")
187188
t.Log(hint)
188189
} else {
189190
t.Log("PASS")
190191
}
191-
192-
interp.CapturedOutput = nil
193192
}()
194193

195194
hint = fmt.Sprintf("To dump SSA representation, run:\n%% go build golang.org/x/tools/cmd/ssadump && ./ssadump -test -build=CFP %s\n", input)
@@ -209,8 +208,6 @@ func run(t *testing.T, input string, goroot string) {
209208
t.Fatalf("not a main package: %s", input)
210209
}
211210

212-
interp.CapturedOutput = new(bytes.Buffer)
213-
214211
sizes := types.SizesFor("gc", ctx.GOARCH)
215212
if sizes.Sizeof(types.Typ[types.Int]) < 4 {
216213
panic("bogus SizesFor")
@@ -222,12 +219,8 @@ func run(t *testing.T, input string, goroot string) {
222219
//
223220
// While capturing is in effect, we must not write any
224221
// test-related stuff to stderr (including log.Print, t.Log, etc).
225-
//
226-
// Suppress capturing if we are the child process of TestRangeFunc.
227-
// TODO(adonovan): simplify that test using this mechanism.
228-
// Also eliminate the redundant interp.CapturedOutput mechanism.
229-
restore := func() {} // restore files and log the mixed out/err.
230-
if os.Getenv("INTERPTEST_CHILD") == "" {
222+
var restore func() string // restore files and log+return the mixed out/err.
223+
{
231224
// Connect std{out,err} to pipe.
232225
r, w, err := os.Pipe()
233226
if err != nil {
@@ -239,7 +232,7 @@ func run(t *testing.T, input string, goroot string) {
239232
os.Stderr = w
240233

241234
// Buffer what is written.
242-
var buf bytes.Buffer
235+
var buf strings.Builder
243236
done := make(chan struct{})
244237
go func() {
245238
if _, err := io.Copy(&buf, r); err != nil {
@@ -249,33 +242,33 @@ func run(t *testing.T, input string, goroot string) {
249242
}()
250243

251244
// Finally, restore the files and log what was captured.
252-
restore = func() {
245+
restore = func() string {
253246
os.Stdout = savedStdout
254247
os.Stderr = savedStderr
255248
w.Close()
256249
<-done
257-
t.Logf("Interpreter's stdout+stderr:\n%s", &buf)
250+
captured := buf.String()
251+
t.Logf("Interpreter's stdout+stderr:\n%s", captured)
252+
return captured
258253
}
259254
}
260255

261256
var imode interp.Mode // default mode
262257
// imode |= interp.DisableRecover // enable for debugging
263258
// imode |= interp.EnableTracing // enable for debugging
264259
exitCode := interp.Interpret(mainPkg, imode, sizes, input, []string{})
265-
restore()
260+
capturedOutput := restore()
266261
if exitCode != 0 {
267262
t.Fatalf("interpreting %s: exit code was %d", input, exitCode)
268263
}
269264
// $GOROOT/test tests use this convention:
270-
if strings.Contains(interp.CapturedOutput.String(), "BUG") {
265+
if strings.Contains(capturedOutput, "BUG") {
271266
t.Fatalf("interpreting %s: exited zero but output contained 'BUG'", input)
272267
}
273268

274269
hint = "" // call off the hounds
275270

276-
if false {
277-
t.Log(input, time.Since(start)) // test profiling
278-
}
271+
return capturedOutput
279272
}
280273

281274
// makeGoroot copies testdata/src into the "src" directory of a temporary
@@ -327,13 +320,9 @@ const GOARCH = %q
327320
// TestTestdataFiles runs the interpreter on testdata/*.go.
328321
func TestTestdataFiles(t *testing.T) {
329322
goroot := makeGoroot(t)
330-
cwd, err := os.Getwd()
331-
if err != nil {
332-
log.Fatal(err)
333-
}
334323
for _, input := range testdataTests {
335324
t.Run(input, func(t *testing.T) {
336-
run(t, filepath.Join(cwd, "testdata", input), goroot)
325+
run(t, filepath.Join("testdata", input), goroot)
337326
})
338327
}
339328
}

‎go/ssa/interp/ops.go

Lines changed: 2 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ import (
1313
"os"
1414
"reflect"
1515
"strings"
16-
"sync"
1716
"unsafe"
1817

1918
"golang.org/x/tools/go/ssa"
@@ -950,27 +949,8 @@ func typeAssert(i *interpreter, instr *ssa.TypeAssert, itf iface) value {
950949
return v
951950
}
952951

953-
// If CapturedOutput is non-nil, all writes by the interpreted program
954-
// to file descriptors 1 and 2 will also be written to CapturedOutput.
955-
//
956-
// (The $GOROOT/test system requires that the test be considered a
957-
// failure if "BUG" appears in the combined stdout/stderr output, even
958-
// if it exits zero. This is a global variable shared by all
959-
// interpreters in the same process.)
952+
// This variable is no longer used but remains to prevent build breakage.
960953
var CapturedOutput *bytes.Buffer
961-
var capturedOutputMu sync.Mutex
962-
963-
// write writes bytes b to the target program's standard output.
964-
// The print/println built-ins and the write() system call funnel
965-
// through here so they can be captured by the test driver.
966-
func print(b []byte) (int, error) {
967-
if CapturedOutput != nil {
968-
capturedOutputMu.Lock()
969-
CapturedOutput.Write(b) // ignore errors
970-
capturedOutputMu.Unlock()
971-
}
972-
return os.Stdout.Write(b)
973-
}
974954

975955
// callBuiltin interprets a call to builtin fn with arguments args,
976956
// returning its result.
@@ -1026,7 +1006,7 @@ func callBuiltin(caller *frame, callpos token.Pos, fn *ssa.Builtin, args []value
10261006
if ln {
10271007
buf.WriteRune('\n')
10281008
}
1029-
print(buf.Bytes())
1009+
os.Stderr.Write(buf.Bytes())
10301010
return nil
10311011

10321012
case "len":

‎go/ssa/interp/rangefunc_test.go

Lines changed: 47 additions & 89 deletions
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,9 @@
55
package interp_test
66

77
import (
8-
"bytes"
9-
"log"
10-
"os"
11-
"os/exec"
128
"path/filepath"
139
"reflect"
10+
"strings"
1411
"testing"
1512

1613
"golang.org/x/tools/internal/testenv"
@@ -19,34 +16,15 @@ import (
1916
func TestIssue69298(t *testing.T) {
2017
testenv.NeedsGo1Point(t, 23)
2118

22-
// TODO: Is cwd actually needed here?
2319
goroot := makeGoroot(t)
24-
cwd, err := os.Getwd()
25-
if err != nil {
26-
log.Fatal(err)
27-
}
28-
run(t, filepath.Join(cwd, "testdata", "fixedbugs/issue69298.go"), goroot)
20+
run(t, filepath.Join("testdata", "fixedbugs", "issue69298.go"), goroot)
2921
}
3022

31-
// TestRangeFunc tests range-over-func in a subprocess.
3223
func TestRangeFunc(t *testing.T) {
3324
testenv.NeedsGo1Point(t, 23)
3425

35-
// TODO(taking): Remove subprocess from the test and capture output another way.
36-
if os.Getenv("INTERPTEST_CHILD") == "1" {
37-
testRangeFunc(t)
38-
return
39-
}
40-
41-
testenv.NeedsExec(t)
42-
testenv.NeedsTool(t, "go")
43-
44-
cmd := exec.Command(os.Args[0], "-test.run=TestRangeFunc")
45-
cmd.Env = append(os.Environ(), "INTERPTEST_CHILD=1")
46-
out, err := cmd.CombinedOutput()
47-
if len(out) > 0 {
48-
t.Logf("out=<<%s>>", out)
49-
}
26+
goroot := makeGoroot(t)
27+
out := run(t, filepath.Join("testdata", "rangefunc.go"), goroot)
5028

5129
// Check the output of the tests.
5230
const (
@@ -62,14 +40,14 @@ func TestRangeFunc(t *testing.T) {
6240
)
6341
expected := map[string][]string{
6442
// rangefunc.go
65-
"TestCheck": []string{"i = 45", CERR_DONE},
66-
"TestCooperativeBadOfSliceIndex": []string{RERR_EXHAUSTED, "i = 36"},
67-
"TestCooperativeBadOfSliceIndexCheck": []string{CERR_EXHAUSTED, "i = 36"},
68-
"TestTrickyIterAll": []string{"i = 36", RERR_EXHAUSTED},
69-
"TestTrickyIterOne": []string{"i = 1", RERR_EXHAUSTED},
70-
"TestTrickyIterZero": []string{"i = 0", RERR_EXHAUSTED},
71-
"TestTrickyIterZeroCheck": []string{"i = 0", CERR_EXHAUSTED},
72-
"TestTrickyIterEcho": []string{
43+
"TestCheck": {"i = 45", CERR_DONE},
44+
"TestCooperativeBadOfSliceIndex": {RERR_EXHAUSTED, "i = 36"},
45+
"TestCooperativeBadOfSliceIndexCheck": {CERR_EXHAUSTED, "i = 36"},
46+
"TestTrickyIterAll": {"i = 36", RERR_EXHAUSTED},
47+
"TestTrickyIterOne": {"i = 1", RERR_EXHAUSTED},
48+
"TestTrickyIterZero": {"i = 0", RERR_EXHAUSTED},
49+
"TestTrickyIterZeroCheck": {"i = 0", CERR_EXHAUSTED},
50+
"TestTrickyIterEcho": {
7351
"first loop i=0",
7452
"first loop i=1",
7553
"first loop i=3",
@@ -79,7 +57,7 @@ func TestRangeFunc(t *testing.T) {
7957
RERR_EXHAUSTED,
8058
"end i=0",
8159
},
82-
"TestTrickyIterEcho2": []string{
60+
"TestTrickyIterEcho2": {
8361
"k=0,x=1,i=0",
8462
"k=0,x=2,i=1",
8563
"k=0,x=3,i=3",
@@ -89,37 +67,37 @@ func TestRangeFunc(t *testing.T) {
8967
RERR_EXHAUSTED,
9068
"end i=1",
9169
},
92-
"TestBreak1": []string{"[1 2 -1 1 2 -2 1 2 -3]"},
93-
"TestBreak2": []string{"[1 2 -1 1 2 -2 1 2 -3]"},
94-
"TestContinue": []string{"[-1 1 2 -2 1 2 -3 1 2 -4]"},
95-
"TestBreak3": []string{"[100 10 2 4 200 10 2 4 20 2 4 300 10 2 4 20 2 4 30]"},
96-
"TestBreak1BadA": []string{"[1 2 -1 1 2 -2 1 2 -3]", RERR_DONE},
97-
"TestBreak1BadB": []string{"[1 2]", RERR_DONE},
98-
"TestMultiCont0": []string{"[1000 10 2 4 2000]"},
99-
"TestMultiCont1": []string{"[1000 10 2 4]", RERR_DONE},
100-
"TestMultiCont2": []string{"[1000 10 2 4]", RERR_DONE},
101-
"TestMultiCont3": []string{"[1000 10 2 4]", RERR_DONE},
102-
"TestMultiBreak0": []string{"[1000 10 2 4]", RERR_DONE},
103-
"TestMultiBreak1": []string{"[1000 10 2 4]", RERR_DONE},
104-
"TestMultiBreak2": []string{"[1000 10 2 4]", RERR_DONE},
105-
"TestMultiBreak3": []string{"[1000 10 2 4]", RERR_DONE},
106-
"TestPanickyIterator1": []string{panickyIterMsg},
107-
"TestPanickyIterator1Check": []string{panickyIterMsg},
108-
"TestPanickyIterator2": []string{RERR_MISSING},
109-
"TestPanickyIterator2Check": []string{CERR_MISSING},
110-
"TestPanickyIterator3": []string{"[100 10 1 2 200 10 1 2]"},
111-
"TestPanickyIterator3Check": []string{"[100 10 1 2 200 10 1 2]"},
112-
"TestPanickyIterator4": []string{RERR_MISSING},
113-
"TestPanickyIterator4Check": []string{CERR_MISSING},
114-
"TestVeryBad1": []string{"[1 10]"},
115-
"TestVeryBad2": []string{"[1 10]"},
116-
"TestVeryBadCheck": []string{"[1 10]"},
117-
"TestOk": []string{"[1 10]"},
118-
"TestBreak1BadDefer": []string{RERR_DONE, "[1 2 -1 1 2 -2 1 2 -3 -30 -20 -10]"},
119-
"TestReturns": []string{"[-1 1 2 -10]", "[-1 1 2 -10]", RERR_DONE, "[-1 1 2 -10]", RERR_DONE},
120-
"TestGotoA": []string{"testGotoA1[-1 1 2 -2 1 2 -3 1 2 -4 -30 -20 -10]", "testGotoA2[-1 1 2 -2 1 2 -3 1 2 -4 -30 -20 -10]", RERR_DONE, "testGotoA3[-1 1 2 -10]", RERR_DONE},
121-
"TestGotoB": []string{"testGotoB1[-1 1 2 999 -10]", "testGotoB2[-1 1 2 -10]", RERR_DONE, "testGotoB3[-1 1 2 -10]", RERR_DONE},
122-
"TestPanicReturns": []string{
70+
"TestBreak1": {"[1 2 -1 1 2 -2 1 2 -3]"},
71+
"TestBreak2": {"[1 2 -1 1 2 -2 1 2 -3]"},
72+
"TestContinue": {"[-1 1 2 -2 1 2 -3 1 2 -4]"},
73+
"TestBreak3": {"[100 10 2 4 200 10 2 4 20 2 4 300 10 2 4 20 2 4 30]"},
74+
"TestBreak1BadA": {"[1 2 -1 1 2 -2 1 2 -3]", RERR_DONE},
75+
"TestBreak1BadB": {"[1 2]", RERR_DONE},
76+
"TestMultiCont0": {"[1000 10 2 4 2000]"},
77+
"TestMultiCont1": {"[1000 10 2 4]", RERR_DONE},
78+
"TestMultiCont2": {"[1000 10 2 4]", RERR_DONE},
79+
"TestMultiCont3": {"[1000 10 2 4]", RERR_DONE},
80+
"TestMultiBreak0": {"[1000 10 2 4]", RERR_DONE},
81+
"TestMultiBreak1": {"[1000 10 2 4]", RERR_DONE},
82+
"TestMultiBreak2": {"[1000 10 2 4]", RERR_DONE},
83+
"TestMultiBreak3": {"[1000 10 2 4]", RERR_DONE},
84+
"TestPanickyIterator1": {panickyIterMsg},
85+
"TestPanickyIterator1Check": {panickyIterMsg},
86+
"TestPanickyIterator2": {RERR_MISSING},
87+
"TestPanickyIterator2Check": {CERR_MISSING},
88+
"TestPanickyIterator3": {"[100 10 1 2 200 10 1 2]"},
89+
"TestPanickyIterator3Check": {"[100 10 1 2 200 10 1 2]"},
90+
"TestPanickyIterator4": {RERR_MISSING},
91+
"TestPanickyIterator4Check": {CERR_MISSING},
92+
"TestVeryBad1": {"[1 10]"},
93+
"TestVeryBad2": {"[1 10]"},
94+
"TestVeryBadCheck": {"[1 10]"},
95+
"TestOk": {"[1 10]"},
96+
"TestBreak1BadDefer": {RERR_DONE, "[1 2 -1 1 2 -2 1 2 -3 -30 -20 -10]"},
97+
"TestReturns": {"[-1 1 2 -10]", "[-1 1 2 -10]", RERR_DONE, "[-1 1 2 -10]", RERR_DONE},
98+
"TestGotoA": {"testGotoA1[-1 1 2 -2 1 2 -3 1 2 -4 -30 -20 -10]", "testGotoA2[-1 1 2 -2 1 2 -3 1 2 -4 -30 -20 -10]", RERR_DONE, "testGotoA3[-1 1 2 -10]", RERR_DONE},
99+
"TestGotoB": {"testGotoB1[-1 1 2 999 -10]", "testGotoB2[-1 1 2 -10]", RERR_DONE, "testGotoB3[-1 1 2 -10]", RERR_DONE},
100+
"TestPanicReturns": {
123101
"Got expected 'f return'",
124102
"Got expected 'g return'",
125103
"Got expected 'h return'",
@@ -130,9 +108,9 @@ func TestRangeFunc(t *testing.T) {
130108
},
131109
}
132110
got := make(map[string][]string)
133-
for _, ln := range bytes.Split(out, []byte("\n")) {
134-
if ind := bytes.Index(ln, []byte(" \t ")); ind >= 0 {
135-
n, m := string(ln[:ind]), string(ln[ind+3:])
111+
for _, ln := range strings.Split(out, "\n") {
112+
if ind := strings.Index(ln, " \t "); ind >= 0 {
113+
n, m := ln[:ind], ln[ind+3:]
136114
got[n] = append(got[n], m)
137115
}
138116
}
@@ -146,24 +124,4 @@ func TestRangeFunc(t *testing.T) {
146124
t.Errorf("No expected output for test %s. got %v", n, gs)
147125
}
148126
}
149-
150-
var exitcode int
151-
if err, ok := err.(*exec.ExitError); ok {
152-
exitcode = err.ExitCode()
153-
}
154-
const want = 0
155-
if exitcode != want {
156-
t.Errorf("exited %d, want %d", exitcode, want)
157-
}
158-
}
159-
160-
func testRangeFunc(t *testing.T) {
161-
goroot := makeGoroot(t)
162-
cwd, err := os.Getwd()
163-
if err != nil {
164-
log.Fatal(err)
165-
}
166-
167-
input := "rangefunc.go"
168-
run(t, filepath.Join(cwd, "testdata", input), goroot)
169127
}

0 commit comments

Comments
 (0)