Merge lp:~rogpeppe/gocheck/add-list-flag into lp:gocheck

[Roger Peppe]

gocheck: add -gocheck.list flag

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

[Roger Peppe (rogpeppe)]

Reviewers: mp+94937_code.launchpad.net,

Message:
Please take a look.

Description:
Also make the filter flag consistent with gotest by naming
it "gocheck.run", and make it easier to see what will
be matched against by matching against the list output.

https://code.launchpad.net/~rogpeppe/gocheck/add-list-flag/+merge/94937

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/5705044/

Affected files:
   M gocheck.go
   M run.go
   M run_test.go

[Gustavo Niemeyer (niemeyer)]

Please separate the two changes. We can't modify .f before the next
announced release.

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

[Roger Peppe (rogpeppe)]

Please take a look.

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

[lp:~rogpeppe/gocheck/add-list-flag]

76. By Roger Peppe on 2012-02-28

revert -gocheck.run to -gocheck.f

[Gustavo Niemeyer (niemeyer)]

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))
We can't change the behavior of .f before the next announced release.

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)")
Please revert this. -gocheck.list is documented below already.

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",
What is this "*" doing there?

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

[Roger Peppe (rogpeppe)]

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: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.

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: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.

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: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.

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

[Gustavo Niemeyer (niemeyer)]

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: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.

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: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. 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.

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: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.

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

[Roger Peppe (rogpeppe)]

Please take a look.

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

[Roger Peppe (rogpeppe)]

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/

[lp:~rogpeppe/gocheck/add-list-flag]

77. By Roger Peppe on 2012-02-28

changes in response to review

78. By Roger Peppe on 2012-02-28

go fmt

[Roger Peppe (rogpeppe)]

Please take a look.

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

[lp:~rogpeppe/gocheck/add-list-flag]

79. By Roger Peppe on 2012-03-14

a little test

[Gustavo Niemeyer (niemeyer)]

LGTM.. I'm merging this with tip and integrating.

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

=== modified file 'bootstrap_test.go'
--- bootstrap_test.go	2011-11-08 15:09:50 +0000
+++ bootstrap_test.go	2012-03-14 14:30:29 +0000
@@ -13,9 +13,9 @@
 package gocheck_test
 
 import (
+	"fmt"
 	"launchpad.net/gocheck"
 	"strings"
-	"fmt"
 )
 
 type BootstrapS struct{}

=== modified file 'checkers.go'
--- checkers.go	2012-02-25 19:01:16 +0000
+++ checkers.go	2012-03-14 14:30:29 +0000
@@ -1,9 +1,9 @@
 package gocheck
 
 import (
+	"fmt"
 	"reflect"
 	"regexp"
-	"fmt"
 )
 
 // -----------------------------------------------------------------------
@@ -238,7 +238,6 @@
 	return value.Len() == n, ""
 }
 
-
 // -----------------------------------------------------------------------
 // ErrorMatches checker.
 

=== modified file 'foundation_test.go'
--- foundation_test.go	2012-02-25 16:58:37 +0000
+++ foundation_test.go	2012-03-14 14:30:29 +0000
@@ -7,12 +7,12 @@
 package gocheck_test
 
 import (
+	"fmt"
 	"launchpad.net/gocheck"
-	"strings"
-	"regexp"
-	"fmt"
 	"log"
 	"os"
+	"regexp"
+	"strings"
 )
 
 // -----------------------------------------------------------------------

=== modified file 'gocheck.go'
--- gocheck.go	2012-02-25 18:28:36 +0000
+++ gocheck.go	2012-03-14 14:30:29 +0000
@@ -53,6 +53,24 @@
 	return method.Info.Func.Pointer()
 }
 
+func (method *methodType) suiteName() string {
+	t := method.Info.Type.In(0)
+	if t.Kind() == reflect.Ptr {
+		t = t.Elem()
+	}
+	return t.Name()
+}
+
+func (method *methodType) String() string {
+	return fmt.Sprintf("%v.%s", method.suiteName(), method.Info.Name)
+}
+
+func (method *methodType) matches(re *regexp.Regexp) bool {
+	return re.MatchString(method.Info.Name) ||
+		re.MatchString(method.suiteName()) ||
+		re.MatchString(method.String())
+}
+
 type C struct {
 	method   *methodType
 	kind     funcKind
@@ -112,7 +130,7 @@
 	if td._path != "" {
 		err := os.RemoveAll(td._path)
 		if err != nil {
-			fmt.Fprintf(os.Stderr, "WARNING: Error cleaning up temporaries: " + err.Error())
+			fmt.Fprintf(os.Stderr, "WARNING: Error cleaning up temporaries: "+err.Error())
 		}
 	}
 }
@@ -190,7 +208,7 @@
 	i := 0
 	n := len(s)
 	for i < n {
-		j := i+1
+		j := i + 1
 		for j < n && s[j-1] != '\n' {
 			j++
 		}
@@ -502,24 +520,8 @@
 		}
 	}
 
-	// This map will be used to filter out duplicated methods.  This
-	// looks like a bug in Go, described on issue 906:
-	// http://code.google.com/p/go/issues/detail?id=906
-	seen := make(map[uintptr]bool, suiteNumMethods)
-
-	// XXX Shouldn't Name() work here? Why does it return an empty string?
-	suiteName := suiteType.String()
-	if index := strings.LastIndex(suiteName, "."); index != -1 {
-		suiteName = suiteName[index+1:]
-	}
-
 	for i := 0; i != suiteNumMethods; i++ {
 		method := newMethod(suiteValue, i)
-		methodPC := method.PC()
-		if _, found := seen[methodPC]; found {
-			continue
-		}
-		seen[methodPC] = true
 		switch method.Info.Name {
 		case "SetUpSuite":
 			runner.setUpSuite = method
@@ -530,7 +532,10 @@
 		case "TearDownTest":
 			runner.tearDownTest = method
 		default:
-			if isWantedTest(suiteName, method.Info.Name, filterRegexp) {
+			if !strings.HasPrefix(method.Info.Name, "Test") {
+				continue
+			}
+			if filterRegexp == nil || method.matches(filterRegexp) {
 				runner.tests[testsLen] = method
 				testsLen += 1
 			}
@@ -541,19 +546,6 @@
 	return runner
 }
 
-// Return true if the given suite name and method name should be
-// considered as a test to be run.
-func isWantedTest(suiteName, testName string, filterRegexp *regexp.Regexp) bool {
-	if !strings.HasPrefix(testName, "Test") {
-		return false
-	} else if filterRegexp == nil {
-		return true
-	}
-	return (filterRegexp.MatchString(testName) ||
-		filterRegexp.MatchString(suiteName) ||
-		filterRegexp.MatchString(suiteName+"."+testName))
-}
-
 // Run all methods in the given suite.
 func (runner *suiteRunner) run() *Result {
 	if runner.tracker.result.RunError == nil && len(runner.tests) > 0 {

=== modified file 'gocheck_test.go'
--- gocheck_test.go	2012-02-17 01:57:02 +0000
+++ gocheck_test.go	2012-03-14 14:30:29 +0000
@@ -4,13 +4,13 @@
 package gocheck_test
 
 import (
-	"launchpad.net/gocheck"
-	"testing"
-	"runtime"
-	"regexp"
 	"flag"
 	"fmt"
+	"launchpad.net/gocheck"
 	"os"
+	"regexp"
+	"runtime"
+	"testing"
 )
 
 // We count the number of suites run at least to get a vague hint that the

=== modified file 'helpers_test.go'
--- helpers_test.go	2012-02-25 18:28:36 +0000
+++ helpers_test.go	2012-03-14 14:30:29 +0000
@@ -126,7 +126,7 @@
 	testHelperFailure(c, "Check(1, checker, 2, msg)", false, false, log,
 		func() interface{} {
 			// Nice leading comment.
-			return c.Check(1, checker, 2)  // Hello there
+			return c.Check(1, checker, 2) // Hello there
 		})
 }
 
@@ -379,7 +379,6 @@
 		})
 }
 
-
 // -----------------------------------------------------------------------
 // MakeDir() tests.
 

=== modified file 'run.go'
--- run.go	2012-02-25 13:01:18 +0000
+++ run.go	2012-03-14 14:30:29 +0000
@@ -1,9 +1,11 @@
 package gocheck
 
 import (
-	"testing"
+	"bufio"
 	"flag"
 	"fmt"
+	"os"
+	"testing"
 )
 
 // -----------------------------------------------------------------------
@@ -23,24 +25,35 @@
 // Public running interface.
 
 var filterFlag = flag.String("gocheck.f", "",
-	"Regular expression selecting what to run")
+	"Regular expression selecting which tests and/or suites to run")
 var verboseFlag = flag.Bool("gocheck.v", false,
 	"Verbose mode")
 var streamFlag = flag.Bool("gocheck.vv", false,
 	"Super verbose mode (disables output caching)")
+var listFlag = flag.Bool("gocheck.list", false,
+	"List the names of all tests that will be run")
 
 // Run all test suites registered with the Suite() function, printing
 // results to stdout, and reporting any failures back to the 'testing'
 // module.
 func TestingT(testingT *testing.T) {
-	result := RunAll(&RunConf{Filter: *filterFlag, Verbose: *verboseFlag, Stream: *streamFlag})
+	conf := &RunConf{Filter: *filterFlag, Verbose: *verboseFlag, Stream: *streamFlag}
+	if *listFlag {
+		w := bufio.NewWriter(os.Stdout)
+		for _, name := range ListAll(conf) {
+			fmt.Fprintf(w, "%s\n", name)
+		}
+		w.Flush()
+		return
+	}
+	result := RunAll(conf)
 	println(result.String())
 	if !result.Passed() {
 		testingT.Fail()
 	}
 }
 
-// Run all test suites registered with the Suite() function, using the
+// RunAll runs all test suites registered with the Suite() function, using the
 // given run configuration.
 func RunAll(runConf *RunConf) *Result {
 	result := Result{}
@@ -50,12 +63,34 @@
 	return &result
 }
 
-// Run the given test suite using the provided run configuration.
+// Run runs the given test suite using the provided run configuration.
 func Run(suite interface{}, runConf *RunConf) *Result {
 	runner := newSuiteRunner(suite, runConf)
 	return runner.run()
 }
 
+// ListAll returns the names of all the test functions registered with the
+// Suite function that will be run with the provided run configuration.
+func ListAll(runConf *RunConf) []string {
+	var names []string
+	for _, suite := range allSuites {
+		names = append(names, List(suite, runConf)...)
+	}
+	return names
+}
+
+// List prints the names of the test functions in the given
+// suite that will be run with the provided run configuration
+// to the given Writer.
+func List(suite interface{}, runConf *RunConf) []string {
+	var names []string
+	runner := newSuiteRunner(suite, runConf)
+	for _, t := range runner.tests {
+		names = append(names, t.String())
+	}
+	return names
+}
+
 // -----------------------------------------------------------------------
 // Result methods.
 

=== modified file 'run_test.go'
--- run_test.go	2012-02-25 13:01:18 +0000
+++ run_test.go	2012-03-14 14:30:29 +0000
@@ -2,6 +2,7 @@
 
 package gocheck_test
 
+
 import (
 	"errors"
 	. "launchpad.net/gocheck"
@@ -99,21 +100,21 @@
 
 func (s *RunS) TestAdd(c *C) {
 	result := &Result{
-		Succeeded: 1,
-		Skipped: 2,
-		Failed: 3,
-		Panicked: 4,
-		FixturePanicked: 5,
-		Missed: 6,
+		Succeeded:        1,
+		Skipped:          2,
+		Failed:           3,
+		Panicked:         4,
+		FixturePanicked:  5,
+		Missed:           6,
 		ExpectedFailures: 7,
 	}
 	result.Add(&Result{
-		Succeeded: 10,
-		Skipped: 20,
-		Failed: 30,
-		Panicked: 40,
-		FixturePanicked: 50,
-		Missed: 60,
+		Succeeded:        10,
+		Skipped:          20,
+		Failed:           30,
+		Panicked:         40,
+		FixturePanicked:  50,
+		Missed:           60,
 		ExpectedFailures: 70,
 	})
 	c.Check(result.Succeeded, Equals, 11)
@@ -280,6 +281,24 @@
 }
 
 // -----------------------------------------------------------------------
+// Verify that List works correctly.
+
+func (s *RunS) TestListFiltered(c *C) {
+	names := List(&FixtureHelper{}, &RunConf{Filter: "1"})
+	c.Assert(names, DeepEquals, []string{
+		"FixtureHelper.Test1",
+	})
+}
+
+func (s *RunS) TestList(c *C) {
+	names := List(&FixtureHelper{}, &RunConf{})
+	c.Assert(names, DeepEquals, []string{
+		"FixtureHelper.Test1",
+		"FixtureHelper.Test2",
+	})
+}
+
+// -----------------------------------------------------------------------
 // Verify that verbose mode prints tests which pass as well. 
 
 func (s *RunS) TestVerboseMode(c *C) {