Add subtests support to template. #28

itsjamie · 2016-10-27T17:11:35Z

Used an if block and duplicated because of wanting to keep the tabbing
consistent. There might be some way to make it so you don't need to
duplicate?

googlebot · 2016-10-27T17:11:37Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

coveralls · 2016-10-27T17:12:33Z

Changes Unknown when pulling fdb7375 on itsjamie:master into * on cweill:master*.

cweill · 2016-10-27T17:31:03Z

I had a quick look over, and looks good so far. Could you please add a couple test files?

googlebot · 2016-10-27T18:54:32Z

CLAs look good, thanks!

coveralls · 2016-10-27T18:55:39Z

Changes Unknown when pulling b6eda04 on itsjamie:master into * on cweill:master*.

cweill

Can you please add at least one test case for the subtests scenario?

Also instead of duplicating the render files, make two files in the render package with the build flags and with a constant for enabling subtests. For example: const enableSubtests = true.

itsjamie · 2016-11-02T23:10:10Z

Hey, sorry about the delay, looks like there are some artifacts in the PR that are unrelated to the final approach I took, which are the duplicated render files you're mentioning. I'll comment where it changes.

itsjamie · 2016-11-02T23:12:36Z

internal/render/render-template.subtests.go

+	return tmpls.ExecuteTemplate(w, "function", struct {
+		*models.Function
+		PrintInputs bool
+		Subtests    bool


New subtest implementation passes a Subtests bool always set to true for Go 1.7.

See my comment above.

coveralls · 2016-11-03T00:57:30Z

Changes Unknown when pulling c1fbe95 on itsjamie:master into * on cweill:master*.

itsjamie · 2016-11-03T01:43:47Z

@cweill I'm struggling with the tests.. when I comment out the line that removes the temporary file that is generated to compare, that file is actually empty. So the error I'm receiving is:

gotests_test.go:512: "Function with neither receiver, parameters, nor results". 
GenerateTests(testdata/test001.go) error = output.Process: imports.Process: 
/var/folders/2h/w7w01bvd3jn2g8t906b0qcdw0000gn/T/gotests_612167568:19:4: missing ',' 
before newline in argument list (and 2 more errors), wantErr false

Here is the models.Function:

&{Name:Foo1 IsExported:true Receiver:<nil> Parameters:[] Results:[] ReturnsError:false}

I would expect to get an error from the TestFunction or Header if the output of the generated test was empty?

coveralls · 2016-11-03T02:00:55Z

Changes Unknown when pulling 4c565bb on itsjamie:master into * on cweill:master*.

cweill · 2016-11-02T23:07:42Z

internal/render/templates/function.tmpl

@@ -42,11 +42,51 @@ func {{.TestName}}(t *testing.T) {
 		// TODO: Add test cases.
 	}
 	for {{if not .IsNaked}} _, tt := {{end}} range tests {
+		{{if .Subtests}}


Please prefix with {{- if

cweill · 2016-11-03T02:42:55Z

internal/render/templates/function.tmpl

@@ -42,11 +42,51 @@ func {{.TestName}}(t *testing.T) {
 		// TODO: Add test cases.
 	}
 	for {{if not .IsNaked}} _, tt := {{end}} range tests {
+		{{if .Subtests}}
+		t.Run(tt.name, func(t *testing.T) {


You should only wrap the code that is different with if .Subtests. Otherwise there is too much duplication.

cweill · 2016-11-03T02:45:38Z

internal/render/render-template.subtests.go

+	return err
+}
+
+func TestFunction(w io.Writer, f *models.Function, printInputs bool) error {


Instead of having two render files, actually you should add another parameter to TestFunction:

func TestFunction(w io.Writer, f *models.Function, printInputs, useSubtests bool) error

This parameter should be passed all the way from the gotests package, and the main file should determine this value. I think it should be enabled by default in Go 1.7, but be disable-able via a flag.

cweill · 2016-11-03T02:45:50Z

internal/render/render-template.subtests.go

+	return tmpls.ExecuteTemplate(w, "function", struct {
+		*models.Function
+		PrintInputs bool
+		Subtests    bool


See my comment above.

cweill · 2016-11-03T02:48:17Z

If you pass isSubtests as a parameter as mentioned above, it will make testing easier in the gotests package. Then you will not need to create a bunch of new test cases for Go 1.7, just one.

Support -subtests flag on command line, defaults to false to maintain backwards compatibility. Setting to true will cause gotests to utilize the Go 1.7 subtests feature via `t.Run`.

coveralls · 2016-11-03T02:54:25Z

Changes Unknown when pulling 7b394c2 on itsjamie:master into * on cweill:master*.

itsjamie · 2016-11-03T02:56:41Z

Oops. I landed pretty close to what you suggested, however I defaulted to false.

I'm not sure why the test is failing to be honest, I ran the generated file in a diff checker and it found no difference. I went through it with my eyes and I couldn't see any difference.

I'll minimize the duplication in the template...

cweill · 2016-11-03T02:58:24Z

Hm, could it be an extra newline at the end of the output?

cweill · 2016-11-03T03:01:25Z

I just saw your latests updates. Mostly looks good except for the one comment about minimizing code duplication in the subtests template.

coveralls · 2016-11-03T03:03:01Z

Changes Unknown when pulling 4dd98ea on itsjamie:master into * on cweill:master*.

coveralls · 2016-11-03T03:03:01Z

Changes Unknown when pulling 4dd98ea on itsjamie:master into * on cweill:master*.

coveralls · 2016-11-03T03:06:02Z

Changes Unknown when pulling 4c514ca on itsjamie:master into * on cweill:master*.

itsjamie · 2016-11-03T03:06:03Z

You were right @cweill, it was a missing newline at the end of the golden.

Tests pass now on my local for 1.7.3 on darwin/amd64.

cweill · 2016-11-03T03:18:26Z

gotests/main.go

@@ -38,6 +40,7 @@ var (
 	exportedFuncs = flag.Bool("exported", false, `generate tests for exported functions and methods. Takes precedence over -only and -all`)
 	allFuncs      = flag.Bool("all", false, "generate tests for all functions and methods")
 	printInputs   = flag.Bool("i", false, "print test inputs in error messages")
+	subtests      = flag.Bool("subtests", false, "generate tests using the Go 1.7 subtests feature")


I think we should enable this by default on Go 1.7. Subtests is one of the key features that makes table driven tests nice to use. As such, we should encourage its usage as much as possible, hence enabling it by default.

We should call the flag nosubtests which defaults to false, but when provided disables subtests.

cweill · 2016-11-03T03:22:30Z

gotests/main.go

@@ -50,6 +53,7 @@ func main() {
 		ExportedFuncs: *exportedFuncs,
 		AllFuncs:      *allFuncs,
 		PrintInputs:   *printInputs,
+		Subtests:      *subtests,


We should use the build protection markup to only include the flag in Go 1.7+. It doesn't make sense to include it in anything below. In Go 1.6 and below, we can just use a const subtests = &false so that this compiles. How does that sound?

itsjamie · 2016-11-03T03:45:49Z

Sounds good to me. I won't get to it tonight but I should tomorrow night.
Thanks for helping.
On Thu, Nov 3, 2016 at 12:22 AM Charles Weill notifications@github.com
wrote:

@cweill commented on this pull request.

In gotests/main.go
#28 (review):

@@ -38,6 +40,7 @@ var (
exportedFuncs = flag.Bool("exported", false, generate tests for exported functions and methods. Takes precedence over -only and -all)
allFuncs = flag.Bool("all", false, "generate tests for all functions and methods")
printInputs = flag.Bool("i", false, "print test inputs in error messages")

subtests = flag.Bool("subtests", false, "generate tests using the Go 1.7 subtests feature")

I think we should enable this by default on Go 1.7. Subtests is one of the
key features that makes table driven tests nice to use. As such, we should
encourage its usage as much as possible, hence enabling it by default.

We should call the flag nosubtests which defaults to false, but when

provided disables subtests.

In gotests/main.go
#28 (review):
@@ -50,6 +53,7 @@ func main() {
ExportedFuncs: *exportedFuncs,
AllFuncs: *allFuncs,
PrintInputs: *printInputs,
Subtests:      *subtests,
We should use the build protection markup to only include the flag in Go
1.7+. It doesn't make sense to include it in anything below. In Go 1.6 and
below, we can just use a const subtests = &false so that this compiles.
How does that sound?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#28 (review), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AB3aqWGCGY2ZaL1f2sO1vQHZGIEDiCQbks5q6VONgaJpZM4Kimh9
.

cweill · 2016-11-03T04:01:51Z

Sounds good. Thank you!

This changes subtests to be enabled by default.

coveralls · 2016-11-04T22:50:30Z

Changes Unknown when pulling 6ae68cc on itsjamie:master into * on cweill:master*.

coveralls · 2016-11-04T22:52:01Z

Changes Unknown when pulling 2fb048f on itsjamie:master into * on cweill:master*.

cweill · 2016-11-07T04:46:14Z

gotests/nosubtests.go

@@ -0,0 +1,7 @@
+// +build !go1.7


I think this file is unnecessary since subtests will default to false.

cweill · 2016-11-07T04:47:45Z

Just one last comment. Otherwise everything looks good to me.

Fixs a logic error where setting nosubtests to true enabled subtests.

coveralls · 2016-11-07T14:02:06Z

Changes Unknown when pulling 67a2920 on itsjamie:master into * on cweill:master*.

coveralls · 2016-11-07T14:06:47Z

Changes Unknown when pulling 802f0b3 on itsjamie:master into * on cweill:master*.

cweill · 2016-11-07T17:55:07Z

Fixes #23.

itsjamie force-pushed the master branch from fdb7375 to b6eda04 Compare October 27, 2016 18:54

cweill reviewed Nov 2, 2016

View reviewed changes

itsjamie commented Nov 2, 2016

View reviewed changes

cweill reviewed Nov 3, 2016

View reviewed changes

cweill added the enhancement label Nov 3, 2016

Add subtests support.

7b394c2

Support -subtests flag on command line, defaults to false to maintain backwards compatibility. Setting to true will cause gotests to utilize the Go 1.7 subtests feature via `t.Run`.

itsjamie force-pushed the master branch from 4c565bb to 7b394c2 Compare November 3, 2016 02:53

Remove duplication on main function.tmpl introduced by subtests.

4dd98ea

Missing newline in golden.

4c514ca

itsjamie mentioned this pull request Nov 3, 2016

Update gotests to support subtests microsoft/vscode-go#597

Closed

cweill reviewed Nov 3, 2016

View reviewed changes

Use -nosubtests flag to disable subtests when Go version is >= 1.7

6ae68cc

This changes subtests to be enabled by default.

Move conditional setting into init.

2fb048f

cweill reviewed Nov 7, 2016

View reviewed changes

gotests/nosubtests.go

@@ -0,0 +1,7 @@

// +build !go1.7

Copy link

Owner

cweill Nov 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this file is unnecessary since subtests will default to false.

Removing nosubtests build for !go1.7.

67a2920

Fixs a logic error where setting nosubtests to true enabled subtests.

Forgot to updated variable name in flag.

802f0b3

cweill merged commit 68fbe27 into cweill:master Nov 7, 2016

This pull request was closed.

Add subtests support to template. #28

Add subtests support to template. #28

Conversation

itsjamie commented Oct 27, 2016

googlebot commented Oct 27, 2016

coveralls commented Oct 27, 2016 • edited Loading

cweill commented Oct 27, 2016

googlebot commented Oct 27, 2016

coveralls commented Oct 27, 2016 • edited Loading

cweill left a comment

Choose a reason for hiding this comment

itsjamie commented Nov 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 3, 2016 • edited Loading

itsjamie commented Nov 3, 2016

coveralls commented Nov 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cweill commented Nov 3, 2016 • edited Loading

coveralls commented Nov 3, 2016 • edited Loading

itsjamie commented Nov 3, 2016

cweill commented Nov 3, 2016 • edited Loading

cweill commented Nov 3, 2016

coveralls commented Nov 3, 2016

coveralls commented Nov 3, 2016

coveralls commented Nov 3, 2016 • edited Loading

itsjamie commented Nov 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itsjamie commented Nov 3, 2016

@cweill commented on this pull request.

provided disables subtests.

cweill commented Nov 3, 2016

coveralls commented Nov 4, 2016

coveralls commented Nov 4, 2016 • edited Loading

Choose a reason for hiding this comment

cweill commented Nov 7, 2016

coveralls commented Nov 7, 2016

coveralls commented Nov 7, 2016 • edited Loading

cweill commented Nov 7, 2016

coveralls commented Oct 27, 2016 •

edited

Loading

coveralls commented Oct 27, 2016 •

edited

Loading

coveralls commented Nov 3, 2016 •

edited

Loading

coveralls commented Nov 3, 2016 •

edited

Loading

cweill commented Nov 3, 2016 •

edited

Loading

coveralls commented Nov 3, 2016 •

edited

Loading

cweill commented Nov 3, 2016 •

edited

Loading

coveralls commented Nov 3, 2016 •

edited

Loading

coveralls commented Nov 4, 2016 •

edited

Loading

coveralls commented Nov 7, 2016 •

edited

Loading