Moving to golden-file pattern for unit tests #215

tro3 · 2017-02-01T20:19:31Z

Moving hard-coded unit test file content into separate files, a la stdlib. The first pass just converts manifest_test.go, in order to get feedback. This PR supercedes #212 for now, as the integration tests were breaking. The unit test content is low-hanging fruit.

tro3 · 2017-02-01T20:51:46Z

Hmm. Having issues with Travis that I don't fully understand. Will remove and try to resubmit after digging.

tro3 · 2017-02-01T21:13:21Z

Ah - no copyright on test/util.go

tro3 · 2017-02-01T21:18:01Z

@sdboyer - let me know if this is an appropriate direction. Note I've added files to your _testdata directory, a test/util.go file for comparing JSON content, and added a field to the test.Helper struct to record the original working directory. (Will be needed for integration tests.) Not yet ready for merge - just looking for feedback. Thx.

freeformz

Some nits, but otherwise +1 from me.

freeformz · 2017-02-02T22:55:01Z

test/test.go

@@ -45,7 +47,8 @@ type Helper struct {

 // NewHelper initializes a new helper for testing.
 func NewHelper(t *testing.T) *Helper {
-	return &Helper{t: t}
+	wd, _ := os.Getwd()


Since we are in a test, this should probably panic if there is an error.

Fair enough

Also, just a note - go test will always set the cwd of the test process to the directory containing the test file that's running.

freeformz · 2017-02-02T22:58:05Z

test/test.go

+		panic(err)
+	}
+	if strings.HasSuffix(src, ".go") {
+		formatted, err := format.Source(content)


We should be writing well formatted code to disk when necessary and not reformatting it. This can lead to issues when tests will need to deal with poorly formatted code.

Agreed. Was copying the existing pattern, but I did add a "style-independent" JSON comparison to test. If we agree that formatting like this makes the test more brittle, I am happy to remove it - just trying to go with the flow, as it were.

For me anyway, when considering .go code, this would surprise me if I had committed poorly formatted, yet valid, go code in order to test a bug with poorly formatted code.

freeformz · 2017-02-02T22:58:15Z

test/test.go

+// GetTestFileReader reads a file form the testdata directory into memory.  src is
+// relative to ./testdata.  Assumes tests take place starting in the cmd/dep
+// directory.
+func (h *Helper) GetTestFileReader(src string) io.Reader {


IMO this should return an *os.FIle instead of reading the whole thing into memory.

Also return an io.ReadCloser so that whomever calls this can defer a Close().

Once this returns an io.ReadCloser, GetTestFileBytes can use this instead.

Agreed on all points

freeformz · 2017-02-02T22:59:38Z

test/test.go

+// TempCopy copies a temporary file from testdata into the temporary directory.
+// dest is relative to the temp directory location, and src is relative to
+// ./testdata.  Assumes tests take place starting in the cmd/dep directory.
+func (h *Helper) TempCopy(dest, src string) {


I don't see this being used anywhere ATM, so please remove.

It will be in the integration tests, where os processes are called. Still wrapping my head around the correct Go way to handle them.

So, I would add that then when it's used.

Yep, or just push the follow-up commits that use it :)

freeformz · 2017-02-02T23:02:43Z

test/test.go

+// GetTestFileString reads a file form the testdata directory into memory.  src is
+// relative to ./testdata.  Assumes tests take place starting in the cmd/dep
+// directory.
+func (h *Helper) GetTestFileString(src string) string {


AFAICT this isn't really needed, just use []bytes.

okay. This is a newbie (ie me) one. If "forget string - use []byte" is the standard, happy to oblige. I'm just trying to minimize future test developer keystrokes. I'm a lazy, lazy man. :-)

Well, where this method is used it seems like you are converting back to a []byte again.

I also find the []byte vs string thing a bit confounding. In general, I try to just follow the rule of "don't convert unless you must." The conversion isn't terribly costly, but it also isn't free, so all other things being equal, I err on the cheaper side.

(However, if you know you have to add data, []byte is definitely better; strings are immutable, so e.g. string concat results in a full copy. Appending to a []byte is just a normal append, though)

sdboyer

Looks like good progress. No flag for updating the golden files yet, it looks like?

sdboyer · 2017-02-03T04:05:32Z

test/test.go

@@ -414,6 +417,47 @@ func (h *Helper) TempFile(path, contents string) {
 	h.Must(ioutil.WriteFile(filepath.Join(h.tempdir, path), bytes, 0644))
 }

+// GetTestFileBytes reads a file form the testdata directory into memory.  src is


nit: s/form/from/

sdboyer · 2017-02-03T04:08:44Z

test/test.go

+	return string(h.GetTestFileBytes(src))
+}
+
+// GetTestFileReader reads a file form the testdata directory into memory.  src is


nit: s/form/from/

sdboyer · 2017-02-03T04:09:10Z

test/test.go

+// GetTestFileReader reads a file form the testdata directory into memory.  src is
+// relative to ./testdata.  Assumes tests take place starting in the cmd/dep
+// directory.
+func (h *Helper) GetTestFileReader(src string) io.Reader {


Agreed on all points

sdboyer · 2017-02-03T04:11:24Z

test/test.go

+// TempCopy copies a temporary file from testdata into the temporary directory.
+// dest is relative to the temp directory location, and src is relative to
+// ./testdata.  Assumes tests take place starting in the cmd/dep directory.
+func (h *Helper) TempCopy(dest, src string) {


Yep, or just push the follow-up commits that use it :)

sdboyer · 2017-02-03T04:18:35Z

test/test.go

+// GetTestFileString reads a file form the testdata directory into memory.  src is
+// relative to ./testdata.  Assumes tests take place starting in the cmd/dep
+// directory.
+func (h *Helper) GetTestFileString(src string) string {


I also find the []byte vs string thing a bit confounding. In general, I try to just follow the rule of "don't convert unless you must." The conversion isn't terribly costly, but it also isn't free, so all other things being equal, I err on the cheaper side.

(However, if you know you have to add data, []byte is definitely better; strings are immutable, so e.g. string concat results in a full copy. Appending to a []byte is just a normal append, though)

sdboyer · 2017-02-03T04:20:38Z

test/test.go

@@ -45,7 +47,8 @@ type Helper struct {

 // NewHelper initializes a new helper for testing.
 func NewHelper(t *testing.T) *Helper {
-	return &Helper{t: t}
+	wd, _ := os.Getwd()


Also, just a note - go test will always set the cwd of the test process to the directory containing the test file that's running.

sdboyer · 2017-02-03T04:23:29Z

test/util.go

+	"encoding/json"
+)
+
+func AreEqualJSON(s1, s2 string) (bool, error) {


Sorta related to the output reformatting discussion - I think it might be better not to have this function, and instead just do a direct == or bytes.Equal() comparison on the golden data vs. the output data. The only way that erroneously mismatched output should be able to happen there is if something in the formatting rules changes. If that happens, though, then IMO it would be expected that the golden output would have to change.

I guess the philosophical question here is - do you want to include the formatting as something to be explicitly tested? Is, for example, the number of spaces of indent on JSON files part of the spec? Historically I've tried to just compare the data, but I'm happy to go whichever way you want. (I was, by the way, getting all errors when comparing to the golden files, originally. Maybe an extra newline or something. I didn't dig as I was adding this anyway.)

I'll leave it in on this next pass, and if you repeat you want it pulled, I'll be happy to do so.

I'd rather a == or bytes.Equal() as well. While white space isn't part of the spec, a change there should be investigated.

If we are going to keep AreEqualJSON though, I'd rather the name be changed to JSONEqual.

Okay. I'll strip out the JSONEqual function and move to ==.

tro3 · 2017-02-03T16:21:00Z

@sdboyer @freeformz - Guys, thanks for the comments; learning as I go. :-) (No pun intended.)
I reduced the test file handlers to:

func (h *Helper) GetTestFile(src string) io.ReadCloser
func (h *Helper) GetTestFileString(src string) string
func (h *Helper) TempCopy(dest, src string)

(Turns out that last was needed in analyzer_test.go.) I left the JSON comparison function in for now, but if you confirm you want it gone, it will happily pull it. @sdboyer - no CI flags on these updates, as they are only aimed at the unit tests.

Please let me know your thoughts.

googlebot · 2017-02-03T22:09:55Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

googlebot · 2017-02-03T22:20:09Z

CLAs look good, thanks!

tro3 · 2017-02-04T04:30:34Z

@sdboyer @freeformz - the AreJSONEqual function has been removed. Let me know if you see any more needed changes. Thanks, guys.

sdboyer · 2017-02-06T20:54:28Z

Looks pretty good! Thanks for sticking with this. Just...

no CI flags on these updates, as they are only aimed at the unit tests.

Hmm, I think there may have been a misunderstanding about what the flag is supposed to be for. (To be clear, the misunderstanding may be on my end - I just learned about this pattern a couple weeks ago).

The flag isn't useful in a CI context - rather, it's a switch that causes the tests to write the generated results to the golden files, rather than comparing against them. That way, the human running the tests can make a selective decision about when the output is known to be good and overwrite the golden file automatically, rather than having to do any tedious and error prone manual copying.

bradleyfalzon · 2017-02-06T21:50:49Z

I believe the convention is to have the testdata directory called testdata not _testdata. I know some files/directories are excluded that begin with _, but I believe this is to avoid editor temporary files. For example, the go project itself has ~48 directories called testdata.

See https://github.com/golang/go/blob/c2bc727f944fe41e1567b8ac1cf306809801bec9/src/cmd/go/alldocs.go#L725-L726

tro3 · 2017-02-06T22:33:05Z

@bradleyfalzon I'm fine either way. I was just using the existing _testdata to start with, but I'm happy to move it over if the Members confirm.

tro3 · 2017-02-06T22:35:23Z

@sdboyer oh I misunderstood "flags" to mean "CI errors". Do you have an example of the flags you'd like to see that you could point me at?

sdboyer · 2017-02-08T14:54:03Z

@tro3 yep yep - straight from stdlib. There's that update flag declared at the top, and then further down (line 101 as of this writing), it's checked, and if enabled, then the golden file is written to rather than compared against.

tro3 · 2017-02-09T01:52:43Z

Okay, I'm on it. I'm also going to rename _testdata to testdata per above and stdlib. That okay?

…-golden-files

sdboyer · 2017-02-09T13:47:44Z

yep, SGTM

tro3 · 2017-02-09T15:40:36Z

Okay, this one is ready to merge. Let me know if you want it squashed. I'm going to go back to test coverage on Lock and Manifest in a different PR.

sdboyer · 2017-02-09T23:59:33Z

This is cropping up a weird error for me on OSX. Once I get that figured out, I'm ready to merge.

freeformz · 2017-02-10T01:45:37Z

analyzer_test.go

-	}
+	h.TempDir("dep")
+	golden := "analyzer/manifest.json"
+	contents := h.GetTestFileString(golden)


If we are going for consistency with other go tools I think contents should be want and b (down below) should be got which IMO also simplifies reading of the ifs. With that said, there is a lot of those cases in the code where this should probably be cleaned up, so it's fine if we do that in a different PR too.

Oh, good call. IMO, we should try to start setting this bar and sticking with it. Sorry, @tro3, I realize this is dragging on. - do you mind making another change?

tro3 · 2017-02-10T15:26:54Z

Not at all. I'll take care of it.

tro3 · 2017-02-10T22:26:37Z

@sdboyer - moved to want/got and folded in some other test coverage increases I was working on

freeformz · 2017-02-11T00:46:54Z

analyzer_test.go

 )

-func TestDeriveManifestAndLock(t *testing.T) {
-	dir, err := ioutil.TempDir("", "dep")
+func TestAnalyzerInfo(t *testing.T) {


FWIW: I am -1 on tests like this. We wrote the same code on both sides and are asserting they are equal.

I agree it's pretty trivial, but it has regression value for analyzer.Info().

I tend to agree with @freeformz, though this also falls under the heading of "write code that's easy to delete," so i'm not too bothered.

So is the call to delete it, then? I'm fine either way

tro3 · 2017-02-11T17:16:56Z

TestAnalyzerInfo removed. Hoping to wrap this one up and move on to integration tests. Let me know if you'd like any more changes.

…-golden-files

sdboyer · 2017-02-13T04:52:15Z

I think we're good here. If that issue keeps recurring on my local (it's quite odd), I'll just clean it up directly.

Converting manifest_test.go to golden-file pattern

b872a85

googlebot added the cla: yes label Feb 1, 2017

tro3 closed this Feb 1, 2017

Adding copyright and tossing extraneous comment

a494331

tro3 reopened this Feb 1, 2017

tro3 added 3 commits February 1, 2017 15:10

Converting lock_test.go to golden-file pattern

40842a6

Converting txn_writer_test.go to golden-file pattern

d322aba

Converting analyzer_test.go to golden-file pattern

a15e2c2

freeformz reviewed Feb 2, 2017

View reviewed changes

sdboyer suggested changes Feb 3, 2017

View reviewed changes

Cleaning up test file functions

54787d1

googlebot added cla: no and removed cla: yes labels Feb 3, 2017

tro3 force-pushed the unit-test-golden-files branch from 09397b0 to 54787d1 Compare February 3, 2017 22:20

googlebot added cla: yes and removed cla: no labels Feb 3, 2017

Removing AreJSONEqual function

82a35c2

tro3 added 2 commits February 8, 2017 18:37

Adding -update test flag for golden files

18efed6

Merge remote-tracking branch 'remotes/upstream/master' into unit-test…

170ff1e

…-golden-files

freeformz reviewed Feb 10, 2017

View reviewed changes

tro3 added 2 commits February 10, 2017 07:39

Increasing coverage for lock and analyzer

1b5e697

Moving to want/got pattern and adding unit test coverage

59437df

tro3 force-pushed the unit-test-golden-files branch from 5401092 to 59437df Compare February 10, 2017 22:25

freeformz reviewed Feb 11, 2017

View reviewed changes

Remove TestAnalyzerInfo() per feedback

cf250cd

tro3 added 2 commits February 11, 2017 09:20

Merge remote-tracking branch 'remotes/upstream/master' into unit-test…

a28e8d9

…-golden-files

Fix test issue after upstream merge

24b9e31

sdboyer approved these changes Feb 13, 2017

View reviewed changes

sdboyer merged commit ef7f681 into golang:master Feb 13, 2017

This was referenced Feb 13, 2017

Use 'golden' files pattern instead of hardcoding test output #204

Closed

Support updating a list of dependencies #235

Merged

tro3 deleted the unit-test-golden-files branch February 13, 2017 15:44

Moving to golden-file pattern for unit tests #215

Moving to golden-file pattern for unit tests #215

Conversation

tro3 commented Feb 1, 2017

tro3 commented Feb 1, 2017

tro3 commented Feb 1, 2017

tro3 commented Feb 1, 2017

freeformz left a comment

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

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

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

tro3 Feb 3, 2017 • 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

tro3 Feb 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdboyer left a comment

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

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

tro3 Feb 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tro3 commented Feb 3, 2017

googlebot commented Feb 3, 2017

googlebot commented Feb 3, 2017

tro3 commented Feb 4, 2017

sdboyer commented Feb 6, 2017

bradleyfalzon commented Feb 6, 2017

tro3 commented Feb 6, 2017 • edited Loading

tro3 commented Feb 6, 2017

sdboyer commented Feb 8, 2017

tro3 commented Feb 9, 2017

sdboyer commented Feb 9, 2017

tro3 commented Feb 9, 2017

sdboyer commented Feb 9, 2017

freeformz Feb 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tro3 commented Feb 10, 2017 via email

tro3 commented Feb 10, 2017

freeformz Feb 11, 2017 • 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

tro3 commented Feb 11, 2017

sdboyer commented Feb 13, 2017

tro3 Feb 3, 2017 •

edited

Loading

tro3 Feb 3, 2017 •

edited

Loading

tro3 Feb 3, 2017 •

edited

Loading

tro3 commented Feb 6, 2017 •

edited

Loading

freeformz Feb 10, 2017 •

edited

Loading

freeformz Feb 11, 2017 •

edited

Loading