code review 6554071: test: Add rundir and errorcheckdir commands to testlib ...

test: Add rundir, rundircmpout and errorcheckdir commands to testlib and run.go

rundir will compile each file in the directory in lexicographic order, link the last file as the main package and run the resulting program. rundircmpout is an related command, that will compare the output of the program to an corresponding .out file

errorcheckdir will compile each file in a directory in lexicographic order, running errorcheck on each file as it compiles. All compilations are assumed to be successful except for the last file. However, If a -0 flag is present on the command, the last compilation will also be assumed successful

This CL also includes a small refactoring of run.go. It was getting unwieldy and the meaning of the run commands was hidden behind argument line formatting.

Fixes issue 4058.

[DMorsing, 2012-09-22 21:58:27]

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

I'd like you to review this change to
https://code.google.com/p/go/

[rsc, 2012-09-23 00:01:05]

Thanks for working on this.

http://codereview.appspot.com/6554071/diff/1002/test/run.go
File test/run.go (right):

http://codereview.appspot.com/6554071/diff/1002/test/run.go#newcode339
test/run.go:339: // "go build" a directory, then run errorcheck on the file
named main.go
There should be a loop here, like there is in compiledir above: we want to
invoke 6g once for each source file. I think for errorcheckdir the rule can be
that the last source file (alphabetically, because we use ioutil.ReadDir) is the
one with the expected errors, or maybe look in each file for "ERROR" to decide
that.

For rundir we can assume that the final source file (again alphabetically) is
package main.

[minux1, 2012-09-23 03:52:18]

On Sunday, September 23, 2012, wrote:
>
> Index: test/testlib
> ==============================**==============================**=======
> --- a/test/testlib
> +++ b/test/testlib
> @@ -51,6 +51,14 @@
>         errchk $G -e $D/$F.go
>  }
>
> +errorcheckdir() {
> +       go run run.go -- $D/$F.go
> +}
> +
> +rundir() {
> +       go run run.go -- $D/$F.go
> +}
> +
>
The testlib changes seem not doing what they suppose to do.
You need a loop here too.

[DMorsing, 2012-09-23 08:33:05]

PTAL

Went with errorcheck on every file in errorcheckdir and last file as main
package in rundir.

Minux: Look closer. I'm implementing the testlib commands by running the run.go
file.

[remyoudompheng, 2012-09-23 17:19:14]

This will allow to enable a bunch of whitelisted tests, but the code duplication
is becoming a bit hard to read, maybe a few helper functions to compile a file,
do errorcheck could make it more digestible?

[rsc, 2012-09-23 18:03:05]

I agree that there are likely some good refactoring possibilities here. Don't be
shy.

Regarding testlib, it would be very nice for it not to run run.go. The reason
for keeping testlib and ./run around is to have something to use when the Go
compiler is suspected of not working. There might not even be a working go
command to invoke. It's a rare case but since testlib is a shell script the code
will probably be shorter than in run.go anyway.

[DMorsing, 2012-09-24 11:34:38]

On Sun, Sep 23, 2012 at 8:03 PM,  <rsc@golang.org> wrote:
> I agree that there are likely some good refactoring possibilities here.
> Don't be shy.
>
> Regarding testlib, it would be very nice for it not to run run.go. The
> reason for keeping testlib and ./run around is to have something to use
> when the Go compiler is suspected of not working. There might not even
> be a working go command to invoke. It's a rare case but since testlib is
> a shell script the code will probably be shorter than in run.go anyway.
>

I agree with Remy. I will do a bit of refactoring while I'm at.

I can see what you mean with regards to testlib. The same issues that
hit self hosting compilers could hit a self hosting test system. Are
there any plans as to whether one of the test systems replaces the
other or are they supposed to be parallel indefinitely?

[remyoudompheng, 2012-09-24 11:38:12]

if the test system is so simple that maintenance of both is free, I
don't see why we should drop any of them, since the shell method has
some advantages reported by rsc. I suggest we examine that when run.go
is complete.

[rsc, 2012-09-24 11:46:30]

> I can see what you mean with regards to testlib. The same issues that
> hit self hosting compilers could hit a self hosting test system. Are
> there any plans as to whether one of the test systems replaces the
> other or are they supposed to be parallel indefinitely?

My plan at least is to keep both indefinitely. The ./run script runs
just one test at a time and has to invoke subshells and scripts for
all the logic. In contrast, run.go can run multiple tests in parallel
and does not fork nearly as many new processes. The end result is that
run.go is quite a bit faster, which is why it's the default in
all.bash. Most of the time testlib is entirely redundant. However,
there are days when everything is broken (for example I changed int to
64 bits on my amd64 system this weekend) and the shell script ./run
lets you at least try the test directory without depending on having a
working Go implementation. On those days, testlib is a huge time
saver.

Russ

[minux1, 2012-09-24 11:47:45]

On Mon, Sep 24, 2012 at 7:34 PM, Daniel Morsing
<daniel.morsing@gmail.com>wrote:

> I can see what you mean with regards to testlib. The same issues that
> hit self hosting compilers could hit a self hosting test system. Are
> there any plans as to whether one of the test systems replaces the
> other or are they supposed to be parallel indefinitely?
>
I think run.go is mainly for the user who is willing to build Go from
source but
not wiling to install extra command line tools just for testing it (e.g.
Windows users)

Whereas testlib and test/run are for the language/runtime developer/porter.
I didn't recognize the importance of testlib and test/run until I tried to
port Go
to FreeBSD/ARM. I didn't know what went wrong, but run.go and cmd/go simply
panicked when ran. I couldn't debug them due to their size and dependencies,
but test/run promptly gave me a small case that triggered the panic.

[DMorsing, 2012-09-24 19:35:59]

PTAL

I have added a short description of the commands in the CL description.

In addition to the testlib changes, I've also refactored run.go a bit in order
to make it more readable.

[DMorsing, 2012-09-26 19:31:17]

ping.

[DMorsing, 2012-10-01 17:59:12]

pinging again.

[iant, 2012-10-01 18:32:49]

https://codereview.appspot.com/6554071/diff/10001/test/run.go
File test/run.go (right):

https://codereview.appspot.com/6554071/diff/10001/test/run.go#newcode168
test/run.go:168: type runCmd func(args ...string) ([]byte, error)
I don't quite see why you are using methods on a runCmd type.  runcmd in
test.run is just a helper function that closes over useTmp and t.tempDir.  I
would probably just pass it in to these additional helper functions.  Also I
wonder if the common sequence
    if err != nil {
        err = fmt.Errorf("%s\n%s", err, out)
    }
should be moved into runcmd.

https://codereview.appspot.com/6554071/diff/10001/test/testlib
File test/testlib (right):

https://codereview.appspot.com/6554071/diff/10001/test/testlib#newcode38
test/testlib:38: if [ -e $D/$F.out ] ; then
Personally, I don't want to see the actions making decisions based on the
existence of a file.  I would rather see rundir and rundircmpout.

[rsc, 2012-10-01 20:17:35]

https://codereview.appspot.com/6554071/diff/10001/test/run.go
File test/run.go (right):

https://codereview.appspot.com/6554071/diff/10001/test/run.go#newcode168
test/run.go:168: type runCmd func(args ...string) ([]byte, error)
On 2012/10/01 18:32:49, iant wrote:
> I don't quite see why you are using methods on a runCmd type.  runcmd in
> test.run is just a helper function that closes over useTmp and t.tempDir.  I
> would probably just pass it in to these additional helper functions.  Also I
> wonder if the common sequence
>     if err != nil {
>         err = fmt.Errorf("%s\n%s", err, out)
>     }
> should be moved into runcmd.

This is definitely a little strange. I understand the motivation for the type,
but instead of defining methods it is probably fine to make runcmd the first
argument to ordinary functions:

func compileFile(runcmd runCmd, longname string) (out []byte, error) {
...
}

etc

https://codereview.appspot.com/6554071/diff/10001/test/testlib
File test/testlib (right):

https://codereview.appspot.com/6554071/diff/10001/test/testlib#newcode34
test/testlib:34: pack grc $name.a $name.$A
Should be able to skip this - import works with .$A files directly.

https://codereview.appspot.com/6554071/diff/10001/test/testlib#newcode38
test/testlib:38: if [ -e $D/$F.out ] ; then
On 2012/10/01 18:32:49, iant wrote:
> Personally, I don't want to see the actions making decisions based on the
> existence of a file.  I would rather see rundir and rundircmpout.

Agreed.

[DMorsing, 2012-10-01 21:03:05]

PTAL

On top of the issues addressed below, I re-added some code that got thrown away
because I did a bad merge. Without it, the errorcheckdir function would not
work.

https://codereview.appspot.com/6554071/diff/10001/test/run.go
File test/run.go (right):

https://codereview.appspot.com/6554071/diff/10001/test/run.go#newcode168
test/run.go:168: type runCmd func(args ...string) ([]byte, error)
On 2012/10/01 20:17:35, rsc wrote:
> On 2012/10/01 18:32:49, iant wrote:
> > I don't quite see why you are using methods on a runCmd type.  runcmd in
> > test.run is just a helper function that closes over useTmp and t.tempDir.  I
> > would probably just pass it in to these additional helper functions.  Also I
> > wonder if the common sequence
> >     if err != nil {
> >         err = fmt.Errorf("%s\n%s", err, out)
> >     }
> > should be moved into runcmd.
> 
> This is definitely a little strange. I understand the motivation for the type,
> but instead of defining methods it is probably fine to make runcmd the first
> argument to ordinary functions:
> 
> func compileFile(runcmd runCmd, longname string) (out []byte, error) {
> ...
> }
> 
> etc

Done.

https://codereview.appspot.com/6554071/diff/10001/test/testlib
File test/testlib (right):

https://codereview.appspot.com/6554071/diff/10001/test/testlib#newcode34
test/testlib:34: pack grc $name.a $name.$A
On 2012/10/01 20:17:35, rsc wrote:
> Should be able to skip this - import works with .$A files directly.

Done.

https://codereview.appspot.com/6554071/diff/10001/test/testlib#newcode38
test/testlib:38: if [ -e $D/$F.out ] ; then
On 2012/10/01 20:17:35, rsc wrote:
> On 2012/10/01 18:32:49, iant wrote:
> > Personally, I don't want to see the actions making decisions based on the
> > existence of a file.  I would rather see rundir and rundircmpout.
> 
> Agreed.

Done.

[iant, 2012-10-01 22:05:04]

https://codereview.appspot.com/6554071/diff/19001/test/run.go
File test/run.go (right):

https://codereview.appspot.com/6554071/diff/19001/test/run.go#newcode171
test/run.go:171: out, err = runcmd("go", "tool", gc, "-e", longname)
Just write
    return runcmd(...)

https://codereview.appspot.com/6554071/diff/19001/test/run.go#newcode176
test/run.go:176: out, err = runcmd("go", "tool", gc, "-e", "-D.", "-I.",
filepath.Join(dir, name))
Here too.

https://codereview.appspot.com/6554071/diff/19001/test/run.go#newcode254
test/run.go:254: err = dirErr
Just write
    return nil, dirErr

https://codereview.appspot.com/6554071/diff/19001/test/run.go#newcode258
test/run.go:258: if filepath.Ext(gofile.Name()) != ".go" {
No reason for a continue statement here, just reverse the condition.
    if filepath.Ext(...) == ".go" {
        filter = ...

https://codereview.appspot.com/6554071/diff/19001/test/run.go#newcode389
test/run.go:389: _, t.err = compileInDir(runcmd, longdir, gofile.Name())
I think you should add
    if t.err != nil {
        break // or return, I suppose
    }

https://codereview.appspot.com/6554071/diff/19001/test/run.go#newcode402
test/run.go:402: out, _ := compileInDir(runcmd, longdir, gofile.Name())
You shouldn't ignore the error from compileInDir.

https://codereview.appspot.com/6554071/diff/19001/test/testlib
File test/testlib (right):

https://codereview.appspot.com/6554071/diff/19001/test/testlib#newcode23
test/testlib:23: grep errorcheck $gofile > /dev/null || zero="-0"
It seems to me that we should pass -0 to all but the last file.  Is that not
right?  Then perhaps errorcheckdir could take a -0 option for whether to pass -0
to the last file.

[DMorsing, 2012-10-02 17:11:24]

PTAL

I have updated the CL description to match the new semantics of the commands.

https://codereview.appspot.com/6554071/diff/19001/test/run.go
File test/run.go (right):

https://codereview.appspot.com/6554071/diff/19001/test/run.go#newcode171
test/run.go:171: out, err = runcmd("go", "tool", gc, "-e", longname)
On 2012/10/01 22:05:04, iant wrote:
> Just write
>     return runcmd(...)

Done.

https://codereview.appspot.com/6554071/diff/19001/test/run.go#newcode176
test/run.go:176: out, err = runcmd("go", "tool", gc, "-e", "-D.", "-I.",
filepath.Join(dir, name))
On 2012/10/01 22:05:04, iant wrote:
> Here too.

Done.

https://codereview.appspot.com/6554071/diff/19001/test/run.go#newcode254
test/run.go:254: err = dirErr
On 2012/10/01 22:05:04, iant wrote:
> Just write
>     return nil, dirErr

Done.

https://codereview.appspot.com/6554071/diff/19001/test/run.go#newcode258
test/run.go:258: if filepath.Ext(gofile.Name()) != ".go" {
On 2012/10/01 22:05:04, iant wrote:
> No reason for a continue statement here, just reverse the condition.
>     if filepath.Ext(...) == ".go" {
>         filter = ...

Done.

https://codereview.appspot.com/6554071/diff/19001/test/run.go#newcode389
test/run.go:389: _, t.err = compileInDir(runcmd, longdir, gofile.Name())
On 2012/10/01 22:05:04, iant wrote:
> I think you should add
>     if t.err != nil {
>         break // or return, I suppose
>     }

Done.

https://codereview.appspot.com/6554071/diff/19001/test/run.go#newcode402
test/run.go:402: out, _ := compileInDir(runcmd, longdir, gofile.Name())
On 2012/10/01 22:05:04, iant wrote:
> You shouldn't ignore the error from compileInDir.

Done.

https://codereview.appspot.com/6554071/diff/19001/test/testlib
File test/testlib (right):

https://codereview.appspot.com/6554071/diff/19001/test/testlib#newcode23
test/testlib:23: grep errorcheck $gofile > /dev/null || zero="-0"
On 2012/10/01 22:05:04, iant wrote:
> It seems to me that we should pass -0 to all but the last file.  Is that not
> right?  Then perhaps errorcheckdir could take a -0 option for whether to pass
-0
> to the last file.

Done.

[rsc, 2012-10-05 20:37:45]

LGTM

[DMorsing, 2012-10-06 07:23:36]

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

test: Add rundir, rundircmpout and errorcheckdir commands to testlib and run.go

rundir will compile each file in the directory in lexicographic order, link the
last file as the main package and run the resulting program. rundircmpout is an
related command, that will compare the output of the program to an corresponding
.out file

errorcheckdir will compile each file in a directory in lexicographic order,
running errorcheck on each file as it compiles. All compilations are assumed to
be successful except for the last file. However, If a -0 flag is present on the
command, the last compilation will also be assumed successful

This CL also includes a small refactoring of run.go. It was getting unwieldy and
the meaning of the run commands was hidden behind argument line formatting.

Fixes issue 4058.

R=rsc, minux.ma, remyoudompheng, iant
CC=golang-dev
http://codereview.appspot.com/6554071