code review 4307049: gotest: another try at flags.

gotest: another try at flags.
doc.go contains the details. The short story:
- command line is passed to the binary
- a new flag, -file, is needed to name files
- known flags have the "test." prefix added for convenience.
- gotest-specific flags are trimmed from the command line.

The effect should be that most existing uses are unaffected,
the ability to name files is still present, and it's nicer to use.
The downside is a lot more code in gotest.

Also allow a test to be called just Test.

[r, 2011-03-30 20:45:30]

Hello rsc, n13m3y3r@gmail.com, rogpeppe@gmail.com (cc:
golang-dev@googlegroups.com),

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

[rsc, 2011-03-30 21:06:00]

http://codereview.appspot.com/4307049/diff/4004/src/cmd/gotest/flag.go
File src/cmd/gotest/flag.go (right):

http://codereview.appspot.com/4307049/diff/4004/src/cmd/gotest/flag.go#newcode25
src/cmd/gotest/flag.go:25: // These flags can be passed with our without a
"test." prefix: -v or -test.v.
s/our/or/

http://codereview.appspot.com/4307049/diff/4004/src/cmd/gotest/flag.go#newcode44
src/cmd/gotest/flag.go:44: hasArg     bool
can drop; hasArg == !isBool

http://codereview.appspot.com/4307049/diff/4004/src/cmd/gotest/flag.go#newcode57
src/cmd/gotest/flag.go:57: // passed to 6.out, adding a "test." prefix to the
name if necessary: -v becomes -test.v.
add bench to list

http://codereview.appspot.com/4307049/diff/4004/src/cmd/gotest/flag.go#newcode61
src/cmd/gotest/flag.go:61: &flagSpec{name: "run", passToTest: true},
missing hasArg: true (but irrelevant if you drop hasArg)

http://codereview.appspot.com/4307049/diff/4004/src/cmd/gotest/flag.go#newcode66
src/cmd/gotest/flag.go:66: // flags processes the command line, grabbing -x and
-c, rewriting all other flags
s/all other/known/

http://codereview.appspot.com/4307049/diff/4004/src/cmd/gotest/flag.go#newcode85
src/cmd/gotest/flag.go:85: if extraWord {
if flag returns value when it consumes extraWord
(as the comment suggests), this can be

filenames = append(filenames, value)

unconditionally

http://codereview.appspot.com/4307049/diff/4004/src/cmd/gotest/flag.go#newcode95
src/cmd/gotest/flag.go:95: arg = strings.TrimLeft(arg, "-")
same here: unconditionally

args = append(args, "-test."+f.name+"="+value)

http://codereview.appspot.com/4307049/diff/4004/src/cmd/gotest/flag.go#newcod...
src/cmd/gotest/flag.go:123: // If there's already "test.", drop it for now.
move up out of loop

http://codereview.appspot.com/4307049/diff/4004/src/cmd/gotest/flag.go#newcod...
src/cmd/gotest/flag.go:136: } else {
Because hasArg == !isBool and because
the doc comment above says it should
return the value, I think this can be

} else {
    extra := equals < 0
    if extra {
        if i+1 >= len(os.Args) {
            usage()
        }
        value = os.Args[i+1]
    }
}

[r2, 2011-03-30 22:11:49]

but what about the approach?

-rob

[r, 2011-03-30 22:23:08]

Hello rsc, niemeyer, rog, r2 (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2011-03-31 00:35:37]

On Wed, Mar 30, 2011 at 18:11, Rob 'Commander' Pike <r@google.com> wrote:
> but what about the approach?

I think the approach is going to work well.

Russ

[rsc, 2011-03-31 00:39:46]

LGTM

http://codereview.appspot.com/4307049/diff/11001/src/cmd/gotest/doc.go
File src/cmd/gotest/doc.go (right):

http://codereview.appspot.com/4307049/diff/11001/src/cmd/gotest/doc.go#newcode39
src/cmd/gotest/doc.go:39: gotest [-file a.go file b.go ...] [-c] [-x] [flags for
test binary]
s2/file/-&/

http://codereview.appspot.com/4307049/diff/11001/src/cmd/gotest/doc.go#newcode43
src/cmd/gotest/doc.go:43: -file f.go add the named file to the package; don't
take *_test.go.
-f file.go  use the tests in the named files instead of *_test.go

http://codereview.appspot.com/4307049/diff/11001/src/cmd/gotest/flag.go
File src/cmd/gotest/flag.go (right):

http://codereview.appspot.com/4307049/diff/11001/src/cmd/gotest/flag.go#newco...
src/cmd/gotest/flag.go:126: extra := equals < 0
s/:=/=/

[niemeyer, 2011-03-31 00:46:11]

LGTM, thanks a lot.

http://codereview.appspot.com/4307049/diff/11001/src/cmd/gotest/flag.go
File src/cmd/gotest/flag.go (right):

http://codereview.appspot.com/4307049/diff/11001/src/cmd/gotest/flag.go#newco...
src/cmd/gotest/flag.go:25: -bench="": passes -test.run bench test
"-test.bench to test"

[r, 2011-03-31 03:49:53]

Hello rsc, niemeyer, rog, r2 (cc: golang-dev@googlegroups.com),

Please take another look.

[r2, 2011-03-31 03:50:23]

all code changes addressed.

i did significant editing to doc.go in this round and would like another pair of
eyes to scan it before i submit.

-rob

[rsc, 2011-03-31 04:08:48]

LGTM

http://codereview.appspot.com/4307049/diff/14001/src/cmd/gotest/doc.go
File src/cmd/gotest/doc.go (right):

http://codereview.appspot.com/4307049/diff/14001/src/cmd/gotest/doc.go#newcode10
src/cmd/gotest/doc.go:10: simple script that recompiles the package whose source
in the current
s/script/tool/

http://codereview.appspot.com/4307049/diff/14001/src/cmd/gotest/doc.go#newcode60
src/cmd/gotest/doc.go:60: If all specified test pass, 6.out prints the word PASS
and exits with
s/test/&s/

http://codereview.appspot.com/4307049/diff/14001/src/cmd/gotest/doc.go#newcode67
src/cmd/gotest/doc.go:67: profile to the specified file when all tests are
complete.
s/when all tests are complete/before exiting/

(change wording because it includes benchmarks)

[r, 2011-03-31 04:14:55]

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

gotest: another try at flags.
doc.go contains the details. The short story:
- command line is passed to the binary
- a new flag, -file, is needed to name files
- known flags have the "test." prefix added for convenience.
- gotest-specific flags are trimmed from the command line.

The effect should be that most existing uses are unaffected,
the ability to name files is still present, and it's nicer to use.
The downside is a lot more code in gotest.

Also allow a test to be called just Test.

R=rsc, niemeyer, rog, r2
CC=golang-dev
http://codereview.appspot.com/4307049

[rog, 2011-03-31 12:27:41]

look good, but i think -- should terminate the
argument processing. perhaps it does and i've
missed it.

On 31 March 2011 05:14,  <r@golang.org> wrote:
> *** Submitted as
> http://code.google.com/p/go/source/detail?r=fc7cf69a3912 ***
>
> gotest: another try at flags.
> doc.go contains the details. The short story:
> - command line is passed to the binary
> - a new flag, -file, is needed to name files
> - known flags have the "test." prefix added for convenience.
> - gotest-specific flags are trimmed from the command line.
>
> The effect should be that most existing uses are unaffected,
> the ability to name files is still present, and it's nicer to use.
> The downside is a lot more code in gotest.
>
> Also allow a test to be called just Test.
>
> R=rsc, niemeyer, rog, r2
> CC=golang-dev
> http://codereview.appspot.com/4307049
>
>
> http://codereview.appspot.com/4307049/
>

[rsc, 2011-03-31 12:55:55]

On Thu, Mar 31, 2011 at 08:27, roger peppe <rogpeppe@gmail.com> wrote:
> look good, but i think -- should terminate the
> argument processing. perhaps it does and i've
> missed it.

It doesn't, and can't, because it doesn't always
know which strings are eaten by options that
precede them.

I believe it is correct as is.

Russ