code review 5625044: test: rewrite test/run shell script + errchk (perl) in Go

test: rewrite test/run shell script + errchk (perl) in Go

This doesn't run all ~750 of the tests, but most.

Progress on issue 2833

[bradfitz, 2012-02-12 08:12:18]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[ality, 2012-02-12 11:21:12]

bradfitz@golang.org once said:
> test: rewrite test/run shell script + errchk (perl) in Go
> 
> This doesn't run all ~750 of the tests, but most.
> The majority use one of a few common shell script patterns.
> 
> This CL doesn't include figure out the final UI, or
> how/whether we'll change the headers of the test files.

I wrote a quick script to figure out which tests
aren't currently supported.  There are 69 of them:

# BEGIN 
64bit.go
	// $G $D/$F.go && $L $F.$A && ./$A.out >tmp.go &&
	// $G tmp.go && $L tmp.$A && ./$A.out
	// rm -f tmp.go

args.go
	// $G $F.go && $L $F.$A && ./$A.out arg1 arg2

bugs/bug395.go
	// echo bug395 is broken

chan/select5.go
	// $G $D/$F.go && $L $F.$A && ./$A.out >tmp.go &&
	// $G tmp.go && $L tmp.$A && ./$A.out
	// rm -f tmp.go

char_lit.go
	// $G $F.go && $L $F.$A &&./$A.out

cmplxdivide.go
	// $G $D/$F.go $D/cmplxdivide1.go && $L $D/$F.$A && ./$A.out

cmplxdivide1.go # missing header

crlf.go
	// $G $D/$F.go && $L $F.$A && ./$A.out >tmp.go &&
	// $G tmp.go && $L tmp.$A && ./$A.out
	// rm -f tmp.go

ddd3.go
	// $G $D/ddd2.go && $G $D/$F.go && $L $F.$A && ./$A.out

dwarf/main.go
	// $G $D/$F.go $D/z*.go && $L $F.$A && ./$A.out

escape2.go
	// errchk -0 $G -m -l $D/$F.go

fixedbugs/bug083.go
	// $G $D/$F.dir/bug0.go && errchk $G $D/$F.dir/bug1.go

fixedbugs/bug282.go
	// $G $D/$F.dir/p1.go && $G $D/$F.dir/p2.go

fixedbugs/bug414.go
	// $G $D/$F.dir/p1.go && $G $D/$F.dir/main.go && $L main.$A && ./$A.out

fixedbugs/bug088.go
	// $G $D/$F.dir/bug0.go && $G $D/$F.dir/bug1.go

fixedbugs/bug401.go
	// $G $D/$F.go && $L $F.$A && ./$A.out || echo "Bug401"

fixedbugs/bug367.go
	// $G $D/$F.dir/p.go && $G $D/$F.dir/main.go && $L main.$A && ./$A.out

fixedbugs/bug382.go
	// $G $D/$F.dir/pkg.go && $G $D/$F.go || echo "Bug 382"

fixedbugs/bug306.go
	// $G $D/$F.dir/p1.go && $G $D/$F.dir/p2.go

fixedbugs/bug106.go
	// $G $D/$F.dir/bug0.go && $G $D/$F.dir/bug1.go

fixedbugs/bug322.go
	// $G $D/$F.dir/lib.go && $G $D/$F.dir/main.go && $L main.$A && ./$A.out

fixedbugs/bug345.go
	// $G $D/$F.dir/io.go && errchk $G -e $D/$F.dir/main.go

fixedbugs/bug038.go
	// ! $G $D/$F.go >/dev/null

fixedbugs/bug392.go
	// $G $D/$F.dir/one.go && $G $D/$F.dir/two.go && $G $D/$F.dir/three.go

fixedbugs/bug415.go
	// $G $D/$F.dir/p.go && $G $D/$F.dir/main.go

fixedbugs/bug036.go
	// ! $G $D/$F.go >/dev/null

fixedbugs/bug040.go
	// ! $G $D/$F.go >/dev/null

fixedbugs/bug385_64.go
	// [ $A != 6 ]  || errchk $G -e $D/$F.go

fixedbugs/bug407.go
	// $G $D/$F.dir/one.go && $G $D/$F.dir/two.go

fixedbugs/bug324.go
	// $G $D/$F.dir/p.go && $G $D/$F.dir/main.go && $L main.$A && ./$A.out

fixedbugs/bug391.go
	// $G $D/$F.go || echo "Issue2576"

fixedbugs/bug369.go
	// $G -N -o slow.$A $D/bug369.dir/pkg.go &&
	// $G -o fast.$A $D/bug369.dir/pkg.go &&

fixedbugs/bug222.go
	// $G $D/$F.dir/chanbug.go && $G -I. $D/$F.dir/chanbug2.go

fixedbugs/bug206.go
	// $G $D/$F.go && $L $F.$A && ./$A.out >/dev/null 2>&1

fixedbugs/bug396.go
	// $G $D/$F.dir/one.go && $G $D/$F.dir/two.go

fixedbugs/bug191.go
	// $G $D/bug191.dir/a.go && $G $D/bug191.dir/b.go && $G $D/$F.go && $L $F.$A

fixedbugs/bug248.go
	// $G $D/$F.dir/bug0.go &&
	// $G $D/$F.dir/bug1.go &&
	// $G $D/$F.dir/bug2.go &&
	// errchk $G -e $D/$F.dir/bug3.go &&
	// $L bug2.$A &&
	// ./$A.out

fixedbugs/bug133.go
	// $G $D/$F.dir/bug0.go && $G $D/$F.dir/bug1.go && errchk $G $D/$F.dir/bug2.go

fixedbugs/bug385_32.go
	// [ $A == 6 ] || errchk $G -e $D/$F.go

fixedbugs/bug399.go
	// $G $D/$F.go || echo "Bug399"

fixedbugs/bug313.go
	// errchk $G -e $D/$F.dir/[ab].go

fixedbugs/bug377.go
	// $G $D/$F.dir/one.go && $G $D/$F.dir/two.go

fixedbugs/bug387.go
	// $G $D/$F.go || echo "Bug387"

fixedbugs/bug404.go
	// $G $D/$F.dir/one.go && $G $D/$F.dir/two.go

fixedbugs/bug302.go
	// $G $D/bug302.dir/p.go && "$GOROOT"/bin/tool/pack grc pp.a p.$A && $G
$D/bug302.dir/main.go

fixedbugs/bug114.go
	// $G $D/$F.go && $L $F.$A && (./$A.out

fixedbugs/bug223.go
	// (! $G $D/$F.go) | grep 'initialization loop' >/dev/null

fixedbugs/bug183.go
	//errchk $G $D/$F.go

fixedbugs/bug160.go
	// $G $D/bug160.dir/x.go && $G $D/bug160.dir/y.go && $L y.$A && ./$A.out

fixedbugs/bug335.go
	// $G $D/$F.dir/b.go && $G $D/$F.dir/a.go
	// rm -f a.$A b.$A

fixedbugs/bug406.go
	// $G $D/$F.go && $L $F.$A && ./$A.out || echo "Bug406"

func1.go
	// errchk $G $F.go

func2.go
	// $G $F.go

func3.go
	// errchk $G $F.go

import3.go
	// $G $D/import2.go && $G $D/$F.go

import4.go
	// $G $D/empty.go && errchk $G $D/$F.go

index.go
	// $G $D/$F.go && $L $F.$A &&
	// ./$A.out -pass 0 >tmp.go && $G tmp.go && $L -o $A.out1 tmp.$A && ./$A.out1
&&
	// ./$A.out -pass 1 >tmp.go && errchk $G -e tmp.go &&
	// ./$A.out -pass 2 >tmp.go && errchk $G -e tmp.go
	// rm -f tmp.go $A.out1

interface/private.go
	// $G $D/${F}1.go && errchk $G $D/$F.go

interface/recursive2.go
	// $G $D/recursive1.go && $G $D/$F.go

interface/embed1.go
	// $G $D/embed0.go && $G $D/$F.go && $L $F.$A && ./$A.out

linkx.go
	// $G $D/$F.go && $L -X main.tbd hello $F.$A && ./$A.out

method4.go
	// $G $D/method4a.go && $G $D/$F.go && $L $F.$A && ./$A.out

nul1.go
	// [ "$GORUN" == "" ] || exit 0
	// $G $D/$F.go && $L $F.$A && ./$A.out >tmp.go &&
	// errchk $G -e tmp.go
	// rm -f tmp.go

safe/usesafe.go
	// $G $D/pkg.go && pack grcS pkg.a pkg.$A 2> /dev/null && rm pkg.$A && $G -I.
-u $D/main.go
	// rm -f pkg.a

safe/nousesafe.go
	// $G $D/pkg.go && pack grc pkg.a pkg.$A 2> /dev/null && rm pkg.$A && errchk $G
-I. -u $D/main.go
	// rm -f pkg.a

sieve.go
	// $G $F.go && $L $F.$A

sigchld.go
	// [ "$GOOS" == windows ] ||
	// ($G $D/$F.go && $L $F.$A && ./$A.out 2>&1 | cmp - $D/$F.out)

sinit.go
	// $G -S $D/$F.go | egrep initdone >/dev/null && echo BUG sinit || true

solitaire.go
	// $G $F.go && $L $F.$A

# END

[rsc, 2012-02-13 17:24:03]

LGTM

Code looks fine, but it could use some comments.
In particular, a comment on each function saying what it does
and how it is expected to be used would be great.

This is fine to check in as a checkpoint, once there are
some function comments.

I suggest (in a separate sequence of CLs) to replace this line in test/run

sed '/^\/\//!q' $i | sed 's@//@@; $d' |sed 's|./\$A.out|$E &|g' >"$RUNFILE"

with

echo '
build() { $G $D/$F.go && $L $F.$A; }
run() { $G $D/$F.go && $L $F.$A && ./$A.out || echo BUG; }
errchk() { ./errchk $G -e $D/$F.go; }
' >"$RUNFILE"
sed '/^\/\//!q' $i | sed 's@//@@; $d' |sed 's|./\$A.out|$E &|g' >>"$RUNFILE"

and then you can start replacing the headers on the tests with
// build, // run, etc.

http://codereview.appspot.com/5625044/diff/9002/test/run.go
File test/run.go (right):

http://codereview.appspot.com/5625044/diff/9002/test/run.go#newcode86
test/run.go:86: if arg == "-" || arg == "--" {
What is this?
If arg == "-" it means someone asked to read from stdin (or something)
and should not be ignored.
If arg == "--" it's even more surprising: "--" ends the flag processing, but the
flag library eats that one.  So you'd have to say go run run.go -- -- in order
to get a -- here.  Delete this statement?

Also it would be nice if this could be a directory here.
Maybe instead

args := flag.Args()
if len(args) == 0 {
    args = dirs
}
for _, arg := range args {
    process arg
}

http://codereview.appspot.com/5625044/diff/9002/test/run.go#newcode107
test/run.go:107: continue
Please print about skipped tests too, to keep us honest.

[r2, 2012-02-13 22:31:50]

Please don't check this in.

I think these tests need to be dealt with a different way. They are a compiler
regression suite and should be marginalized rather than institutionalized.  I
will come up with an approach I prefer.

-rob

[bradfitz, 2012-02-17 04:08:00]

On Tue, Feb 14, 2012 at 4:24 AM, <rsc@golang.org> wrote:

> LGTM
>
> Code looks fine, but it could use some comments.
> In particular, a comment on each function saying what it does
> and how it is expected to be used would be great.
>
> This is fine to check in as a checkpoint, once there are
> some function comments.
>
> I suggest (in a separate sequence of CLs) to replace this line in
> test/run
>
> sed '/^\/\//!q' $i | sed 's@//@@; $d' |sed 's|./\$A.out|$E &|g'
>
>> "$RUNFILE"
>>
>
> with
>
> echo '
> build() { $G $D/$F.go && $L $F.$A; }
> run() { $G $D/$F.go && $L $F.$A && ./$A.out || echo BUG; }
> errchk() { ./errchk $G -e $D/$F.go; }
> ' >"$RUNFILE"
> sed '/^\/\//!q' $i | sed 's@//@@; $d' |sed 's|./\$A.out|$E &|g'
>
>> "$RUNFILE"
>>>
>>
> and then you can start replacing the headers on the tests with
> // build, // run, etc.
>

I failed at this step, several times.  Gmail's formatting didn't help,
either.

Shell and/or sed has defeated me.

I called in reinforcements, but sed was still too much for us.

Can you modify test/run to add the headers?  I can modify the test files in
a batch later.

[rsc, 2012-02-17 04:10:29]

sure, will send a cl

[bradfitz, 2012-02-20 05:45:10]

Hello golang-dev@googlegroups.com, ality@pbrane.org, rsc@golang.org,
r@google.com (cc: golang-dev@googlegroups.com),

Please take another look.

[r, 2012-02-20 06:05:09]

http://codereview.appspot.com/5625044/diff/15001/test/run.go
File test/run.go (right):

http://codereview.appspot.com/5625044/diff/15001/test/run.go#newcode79
test/run.go:79: log.Fatalf("can't yet deal with non-go argument %q", arg)
s/argument/file/

http://codereview.appspot.com/5625044/diff/15001/test/run.go#newcode133
test/run.go:133: if !strings.HasSuffix(names[i], ".go") {
i had to stare at this for a while. simpler:

dirnames, err := f.Readdirnames(-1)
check(err)
for _, name := range dirnames {
  if strings.HasSuffix(name, ".go") {
    names = append(names, name)
  }
}

http://codereview.appspot.com/5625044/diff/15001/test/run.go#newcode160
test/run.go:160: toRun = make(chan *test, 5000)
where does 5000 come from?

http://codereview.appspot.com/5625044/diff/15001/test/run.go#newcode190
test/run.go:190: t.err = skipError("starts with newline")
all these errors needs to include the file name in the printout. it looks like
if check does the work, it won't always be there.

a setError method would do it.
return t.setErr(...)

also t.Errorf would be handy

http://codereview.appspot.com/5625044/diff/15001/test/run.go#newcode281
test/run.go:281: type test struct {
put the type declaration above the methods

[bradfitz, 2012-02-21 00:50:10]

http://codereview.appspot.com/5625044/diff/15001/test/run.go
File test/run.go (right):

http://codereview.appspot.com/5625044/diff/15001/test/run.go#newcode79
test/run.go:79: log.Fatalf("can't yet deal with non-go argument %q", arg)
On 2012/02/20 06:05:09, r wrote:
> s/argument/file/

Done.

http://codereview.appspot.com/5625044/diff/15001/test/run.go#newcode133
test/run.go:133: if !strings.HasSuffix(names[i], ".go") {
On 2012/02/20 06:05:09, r wrote:
> i had to stare at this for a while. simpler:
> 
> dirnames, err := f.Readdirnames(-1)
> check(err)
> for _, name := range dirnames {
>   if strings.HasSuffix(name, ".go") {
>     names = append(names, name)
>   }
> }

Done.

http://codereview.appspot.com/5625044/diff/15001/test/run.go#newcode160
test/run.go:160: toRun = make(chan *test, 5000)
On 2012/02/20 06:05:09, r wrote:
> where does 5000 come from?

made it a constant & documented it.

http://codereview.appspot.com/5625044/diff/15001/test/run.go#newcode190
test/run.go:190: t.err = skipError("starts with newline")
On 2012/02/20 06:05:09, r wrote:
> all these errors needs to include the file name in the printout. it looks like
> if check does the work, it won't always be there.
> 
> a setError method would do it.
> return t.setErr(...)
> 
> also t.Errorf would be handy

Context is included by the caller.

The lazy check() isn't used for t.err.

http://codereview.appspot.com/5625044/diff/15001/test/run.go#newcode281
test/run.go:281: type test struct {
On 2012/02/20 06:05:09, r wrote:
> put the type declaration above the methods

Done.

[bradfitz, 2012-02-21 00:50:13]

Hello golang-dev@googlegroups.com, ality@pbrane.org, rsc@golang.org,
r@google.com, r@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[r, 2012-02-21 03:09:15]

CL description needs update
LGTM as a checkpoint.

http://codereview.appspot.com/5625044/diff/19001/test/run.go
File test/run.go (right):

http://codereview.appspot.com/5625044/diff/19001/test/run.go#newcode56
test/run.go:56: // maxTests is an upper bound on the total number of total
tests.
"total number of total tests"
totally delete one total, preferably the second.

[bradfitz, 2012-02-21 03:28:57]

*** Submitted as http://code.google.com/p/go/source/detail?r=8ae6dac3df56 ***

test: rewrite test/run shell script + errchk (perl) in Go

This doesn't run all ~750 of the tests, but most.

Progress on issue 2833

R=golang-dev, ality, rsc, r, r
CC=golang-dev
http://codereview.appspot.com/5625044