Code review comment for lp:~rogpeppe/gocheck/add-list-flag

Revision history for this message
Roger Peppe (rogpeppe) wrote :

PTAL

https://codereview.appspot.com/5705044/diff/2002/gocheck.go
File gocheck.go (left):

https://codereview.appspot.com/5705044/diff/2002/gocheck.go#oldcode554
gocheck.go:554: filterRegexp.MatchString(suiteName+"."+testName))
On 2012/02/28 14:45:03, niemeyer wrote:
> On 2012/02/28 14:37:20, rog wrote:
> > On 2012/02/28 14:17:27, niemeyer wrote:
> > > We can't change the behavior of .f before the next announced
release.
> >
> > FWIW the new behaviour is exactly the old behaviour if the user
doesn't use ^
> or
> > $ in their regexp. none of the tests needed to change, which is a
useful
> > indicator.
> >
> > i'll revert nonetheless.

> The new behavior is exactly the old behavior except when it's not.

i've reverted to the old behaviour, but left the implementation similar
to this, so that the simplification can be made easily later.

https://codereview.appspot.com/5705044/diff/2002/run.go
File run.go (right):

https://codereview.appspot.com/5705044/diff/2002/run.go#newcode28
run.go:28: "Regular expression selecting what to run (use -gocheck.list
to see what this matches against)")
On 2012/02/28 14:45:03, niemeyer wrote:
> On 2012/02/28 14:37:20, rog wrote:
> > On 2012/02/28 14:17:27, niemeyer wrote:
> > > Please revert this. -gocheck.list is documented below already.
> >
> > AFAIK the text that the regexp matches is not documented anywhere
(for
> instance
> > i didn't know that you could filter on a suite name until i read the
source
> > code). if this isn't a reasonable place to document the behaviour,
perhaps you
> > could suggest another.

> Documenting the -gocheck.list option twice isn't a good way to do what
you want.

that wasn't what i was trying to do. i was trying to document that the
gocheck.f flag matches against the text produced by list, not what list
does.

i like the idea that {go test -gocheck.list | grep pat} will print
exactly the same tests that {go test -gocheck.f pat} will run.

> You can say something like:

> "Regular expression selecting which test and/or suite to run"

> Also, when you see a test name in a verbose run or on a failure, that
format
> works, so it's not like it's entirely magical.

the fact that it tries the regexp against three pieces of text was
unexpected to me. i hope that you think it's a reasonable idea in the
future to standardise on a single form of text to match against - easier
to document and to use IMHO.

https://codereview.appspot.com/5705044/diff/2002/run_test.go
File run_test.go (right):

https://codereview.appspot.com/5705044/diff/2002/run_test.go#newcode296
run_test.go:296: "*gocheck_test.FixtureHelper.Test2",
On 2012/02/28 14:45:03, niemeyer wrote:
> On 2012/02/28 14:37:20, rog wrote:
> > On 2012/02/28 14:17:27, niemeyer wrote:
> > > What is this "*" doing there?
> >
> > the method is on a pointer type. i've found this info useful in the
past.
> > perhaps it shouldn't be indicated though.

> Yeah, not relevant here.

Done.

https://codereview.appspot.com/5705044/

« Back to merge proposal