Integration test harness #287

tro3 · 2017-03-05T15:58:56Z

Addressing #282. @sdboyer, please let me know what you think of this path.

tro3 · 2017-03-05T16:03:32Z

Note that for now, I have the tests under testdata/harness_tests, with only the init tests converted. The whole thing currently runs in parallel to the original suite. Final version will remove the other tests and promote the harness_tests directory one level.

tro3 · 2017-03-06T03:06:55Z

Okay - I set up a vendor initialization function, so I think we can really beat the he** out of this thing now. If the community agrees with the direction, then I will document the structure and usage, and propose a test list to start working against (which would also serve as a high-level functionality cheat sheet).

All thoughts welcome.

tro3 · 2017-03-06T16:11:33Z

This is almost ready to merge, after review and documentation. Increased number of remove tests makes it harder to get past AppVeyor. After this, I will put together a test table porposal in a separate issue for discussion/feedback before filling out the tests.

tro3 · 2017-03-06T16:13:53Z

@sdboyer - where would you like the harness docs? test, cmd/dep, or cmd/dep/testdata?

sdboyer · 2017-03-07T04:20:01Z

@tro3 my first impulse is to say cmd/dep/testdata for the docs.

I've got reviewing this on my TODO list for tomorrow.

tro3 · 2017-03-07T13:20:12Z

Cool. I do think I'm going to move initial/vendors.txt and final/vendors.txt to the root test case directory, as initial_vendors.txt and final_vendors.txt. I found the current structure a little cumbersome for setting up new tests. Let me know if you think otherwise.

tro3 · 2017-03-07T15:46:53Z

Maybe even better would be a single testcase.yaml file:

# ensure/update test case 4 - description here

commands:
  - init
  - ensure -update

imports:
    github.com/sdboyer/deptestdos: 1.0.0

initialVendors:
    github.com/sdboyer/deptest: 0.8.0

finalVendors:
    - github.com/sdboyer/deptest
    - github.com/sdboyer/deptestdos

Then the overall testcase would be a single file to describe it and the initial and final directories to populate and check against. I don't want to require a go-yaml package, so I'd write a simple parser for the above subset. Thoughts?

# Conflicts: # cmd/dep/remove_test.go

tro3 · 2017-03-07T22:04:04Z

Okay, I moved to the yaml config file, blocked the remove tests on Windows, and all seems to be working. Starting to work on documentation.

sdboyer · 2017-03-08T03:39:53Z

~~I actually blocked the remove tests on windows a bit earlier back on aster, so you can pull that part out.~~

nvm this, ofc, the logic has to be a bit different now. You'll still need to rebase back onto master to fix the conflict, though.

sdboyer · 2017-03-08T03:40:00Z

(starting review)

sdboyer

Oi, this is a ton of work - thank you! Overall, this is looking really good! Biggest comment is switching from YAML -> JSON - see inline comment.

sdboyer · 2017-03-08T03:51:27Z

cmd/dep/integration_test.go

+	test.NeedsExternalNetwork(t)
+	test.NeedsGit(t)
+
+	filepath.Walk("testdata", func(path string, info os.FileInfo, err error) error {


Let's actually put an extra parent directory in here - testdata/harness_tests is fine (sorry I didn't catch you earlier with this), so that it's easily possible to use the testdata dir for something other than just this.

Will do. Note that it already can be used for other things. The system just looks for testcase.yaml, so other directories can exist in parallel. But I have no problem moving them back.

Fair enough, but yeah, I think the extra separator is still worthwhile. Given that we're defining semantics around filesystem organization, I think it's better to have the explicit top-level choice.

sdboyer · 2017-03-08T03:59:38Z

test/miniyaml.go

+}
+
+// Only parses key:val pairs (both strings) and lists of strings
+func ParseYaml(rdr io.Reader) (YamlDoc, error) {


Let's please use JSON, instead.

I realize that JSON is suboptimal for this purpose, but it's in stdlib, and until we come to a decision in #119, it's not clear what parsers for more human-readable formats we'll have available to us. I'd rather not introduce an extra dependency for just this purpose, and I like introducing a custom parser even less than that.

Also, would I be right to guess that this implementation probably came from somewhere else?

No, I wrote it on the fly (looking sheepish). No problem - I'll convert to JSON.

ach, sorry, i wish i'd caught it before you put all the time in on that - sorry! thanks for making the change.

sdboyer · 2017-03-08T04:03:34Z

test/int_testcase.go

@@ -0,0 +1,269 @@
+// Copyright 2017 The Go Authors. All rights reserved.


no need to abbreviate filename - integration_testcase.go is fine

sdboyer · 2017-03-08T04:05:20Z

test/int_testcase.go

+	if err != nil {
+		panic(err)
+	}
+	rootPath := filepath.Join(


shorter:

rootPath := filepath.FromSlash(filepath.Join(wd, "testdata", name))

sdboyer · 2017-03-08T04:06:40Z

test/int_testcase.go

+}
+
+//
+// func (tc *IntegrationTestCase) GetImports() map[string]string {


What are these commented bits?

Stupidity. Will remove

tro3 · 2017-03-08T22:43:26Z

@sdboyer, harness feedback implemented

sdboyer

Sooo close! Just a couple of nits.

sdboyer · 2017-03-09T01:18:02Z

cmd/dep/integration_test.go

@@ -18,51 +18,52 @@ func TestIntegration(t *testing.T) {
 	test.NeedsExternalNetwork(t)
 	test.NeedsGit(t)

-	filepath.Walk("testdata", func(path string, info os.FileInfo, err error) error {
+	filepath.Walk(filepath.Join("testdata", "harness_tests"),
+		func(path string, info os.FileInfo, err error) error {


I think that everything on one line here is fine - even preferable, because it'll de-dent everything below. Go is lax about the 80-width column thing.

If the width thing really bugs you, assign the func to a var and then call the filepath.Walk() below with it 😄

sdboyer · 2017-03-09T01:20:21Z

cmd/dep/integration_test.go

-			testName := strings.Join(parse[1:len(parse)-1], "/")
+			if strings.HasSuffix(path, "testcase.json") {
+				parse := strings.Split(path, string(filepath.Separator))
+				testName := strings.Join(parse[2:len(parse)-1], "/")


I think you want filepath.Base() here

sdboyer · 2017-03-09T01:23:40Z

cmd/dep/testdata/harness_tests/README.md

-    finalVendors:
-        - github.com/sdboyer/deptest
-        - github.com/sdboyer/deptestdos
+The test code itself simply walks down the directory tree, looking for `testcase.json` files.  These files can be as many levels down the tree as desired.  The test name will consist of the directory path from `testdata` to the test case directory itself.  In the above example, the test name would be `category1/subcategory1/case1`, and could be singled out with the `-run` option of `go test` (i.e. `go test github.com/golang/dep/cmp/dep -run Integration/category1/subcategory1/case1`).  New tests can be added simply by adding a new directory with the json file to the `testdata` tree.  There is no need for code modification - the new test will be included automatically.


While wrap may not matter overmuch, I think it'd be worth wrapping these at 80; this is a file that likely will be read in a terminal/editors.

sdboyer · 2017-03-09T01:25:41Z

cmd/dep/testdata/harness_tests/README.md

@@ -51,7 +54,7 @@ The test procedure is as follows:
 2. Create `src/github.com/golang/notexist` as the project
 3. Copy the contents of `initial` to the project
 4. `go get` the repos and versions in `imports` to the `src` directory


Let's say fetch rather than go get; less misleading.

Also, should probably note in here that we only support git repos.

tro3 · 2017-03-09T04:51:09Z

@sdboyer - latest mods in place. Pls have a look.

groob

made a few small style nits. Hope you don't mind.

groob · 2017-03-09T04:57:14Z

test/integration_testcase.go

+				if err := tc.WriteFile(golden, got); err != nil {
+					tc.t.Fatal(err)
+				}
+			} else {


Do you need the else for cases where you call t.Fatal first?
It would improve readability of this test if some of the if ... else if ... else code could be simplified into something with fewer indentation levels.

Agreed on the readability here, but don't see an obvious simplification. There is a 2x3 decision matrix being handled, and some of the quadrants need error handling built in, causing more ifs. Open to suggestions - I am a Go newbie.

groob · 2017-03-09T04:57:39Z

test/integration_testcase.go

+
+	gotExists, got, err := getFile(working)
+	if err != nil {
+		panic(err)


Any reason for calling panic instead of Fatal here?

groob · 2017-03-09T04:58:07Z

test/integration_testproj.go

+)
+
+var (
+	ProjectRoot string = "src/github.com/golang/notexist"


Should this var be a const?

Yup. Newbie move.

groob · 2017-03-09T05:01:02Z

test/integration_testcase.go

+	UpdateGolden *bool = flag.Bool("update", false, "update golden files")
+)
+
+// To manage a test case directory structure and content


Use the recommended comment format from go lint here: IntegrationTestCase ...

tro3 · 2017-03-09T05:35:30Z

@groob, thanks for the feedback. Mods are pushed - please let me know what you think.

sdboyer · 2017-03-09T16:06:21Z

@groob thanks for the extra review!

still needs a rebase to fix conflicts :)

tro3 · 2017-03-09T16:42:16Z

? It says it specifically can't be rebased, due to conflicts (files being deleted/moved across branches). But I think merge is ok? I just did it locally without issue. If it does need a rebase, just let me know to where.

sdboyer · 2017-03-09T17:44:11Z

right, it says GH can't rebase because the repository is set to incorporate PRs via rebase, rather than via merge - and there's a conflict. (i just verified the conflict locally - at least one, in cmd/dep/remove_test.go).

i realize it doesn't report the same conflict on merge, but that's not how we're incorporating PRs at the moment, so you'll need to resolve the rebase conflict, push up the rebased branch, and then we can hit the button here.

sdboyer · 2017-03-09T17:50:54Z

oh wait - i can just do the merge here :) thanks!

Integration test harness

tro3 added 3 commits March 5, 2017 06:20

Initial harness structure

96e3291

Conversion of init tests

286fa00

Remove superfluous function

09b9063

googlebot added the cla: yes label Mar 5, 2017

tro3 changed the title ~~Test harness~~ Integration test harness Mar 5, 2017

tro3 changed the title ~~Integration test harness~~ Integration test harness [WIP] Mar 5, 2017

Converting the remainder of tests

237c526

tro3 mentioned this pull request Mar 6, 2017

Abstract integration test harness #282

Closed

Adding initial vendor state and test coverage

46bf6dc

Remove old tests and promote new hierarchy one level

5e0704f

tro3 changed the title ~~Integration test harness [WIP]~~ Integration test harness Mar 6, 2017

tro3 added 3 commits March 7, 2017 12:42

Upgrading test case descriptions to yaml-lite

ca96f26

Merge branch 'master' of https://github.com/golang/dep into test_harness

989ca13

# Conflicts: # cmd/dep/remove_test.go

Removing remove tests on Windows for now

e6f88bf

Adding documentation to cmd/dep/testdata

b14bf14

sdboyer reviewed Mar 8, 2017

View reviewed changes

sdboyer mentioned this pull request Mar 8, 2017

Use new gps capabilities during init's dep search #305

Merged

tro3 added 3 commits March 8, 2017 06:55

Move from miniyaml to json for testcases and other feedback

cd1ea89

Upgrade harness mashalling

717bef7

Remove miniyaml

af0e542

Doc typo

d03ace9

sdboyer suggested changes Mar 9, 2017

View reviewed changes

Review feedback mods

f2b732f

groob reviewed Mar 9, 2017

View reviewed changes

More feedback updates

d3c1115

domgreen mentioned this pull request Mar 9, 2017

fix dep silently failing #307

Closed

sdboyer approved these changes Mar 9, 2017

View reviewed changes

sdboyer merged commit 5b98ab1 into golang:master Mar 9, 2017

sdboyer mentioned this pull request Mar 9, 2017

Respect dry run flag during dep ensure #256

Merged

tro3 deleted the test_harness branch March 9, 2017 18:47

ibrasho pushed a commit to ibrasho-forks/dep that referenced this pull request May 10, 2017

Merge pull request golang#287 from tro3/test_harness

71a1436

Integration test harness

		@@ -0,0 +1,269 @@
		// Copyright 2017 The Go Authors. All rights reserved.

Integration test harness #287

Integration test harness #287

Uh oh!

Conversation

tro3 commented Mar 5, 2017

Uh oh!

tro3 commented Mar 5, 2017

Uh oh!

tro3 commented Mar 6, 2017

Uh oh!

tro3 commented Mar 6, 2017

Uh oh!

tro3 commented Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sdboyer commented Mar 7, 2017

Uh oh!

tro3 commented Mar 7, 2017 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tro3 commented Mar 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tro3 commented Mar 7, 2017

Uh oh!

sdboyer commented Mar 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sdboyer commented Mar 8, 2017

Uh oh!

sdboyer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tro3 commented Mar 8, 2017

Uh oh!

sdboyer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tro3 commented Mar 9, 2017

Uh oh!

groob left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

tro3 commented Mar 6, 2017 •

edited

Loading

tro3 commented Mar 7, 2017 via email •

edited

Loading

tro3 commented Mar 7, 2017 •

edited

Loading

sdboyer commented Mar 8, 2017 •

edited

Loading

tro3 commented Mar 9, 2017 •

edited

Loading