test harness should be able to expect errors #319

domgreen · 2017-03-12T11:53:40Z

Initial PR was #317 I seemed to have messed up my branch 😢

domgreen · 2017-03-12T12:01:39Z

@tro3 thanks for the feedback, have moved to this PR as I did something wrong with git.

I feel as though we should have a test case for the scenario when you have a manifest file already when calling init however from looking at the logs:

--- FAIL: TestIntegration (0.53s)
    --- FAIL: TestIntegration/ensure/update/case1 (0.29s)
    	test.go:157: running testdep [init]
    	test.go:179: standard error:
    	test.go:180: manifest file "/private/var/folders/q9/jmtltdhs5hd8574dpdqgn40c0000gn/T/gotest122542319/src/github.com/golang/notexist/manifest.json" already exists

    	integration_testcase.go:71: exit status 1 error raised where none was expected
FAIL
FAIL	github.com/golang/dep/cmd/dep	7.486s

It looks like stderr gets the correct error message then dep exits with exit status 1 hiding the error I would expect.

domgreen · 2017-03-12T13:36:47Z

Actually, I think I need to look at the output of Stderr rather than the error as dep correctly exists with exit code 1 when a command fails. I think we should still break when an error in any command occurs though. Have set up a test case for this scenario. Will look into it this afternoon.

tro3 · 2017-03-12T13:52:21Z

Agreed that parsing stderr would be preferable, if robust. Perhaps it would make sense to merge this as a first pass, then I could update my PR to include the error handling json update. With both merged, the stderr parsing could be a new PR. @sdboyer, thoughts?

sdboyer · 2017-03-12T14:28:36Z

cmd/dep/testdata/harness_tests/README.md

@@ -57,7 +57,8 @@ The `testcase.json` file has the following format:
        "github.com/sdboyer/deptestdos",
        "github.com/sdboyer/deptesttres",
        "github.com/sdboyer/deptestquatro"
-      ]
+      ],
+      "error-expected":"something went wrong"


nit: space between colon and initial quote around value

sdboyer · 2017-03-12T14:34:30Z

Seems to me like right thing to do here is to only allow for an error on the final command in the list. If we don't make that assumption, then we pretty much have to allow for multiple errors to be specified.

If we do think we should allow errors from commands other than the final one, then rather than adding a new key to the JSON format, I think I'd prefer to see the command declaration elements themselves become 2-tuples, where the first element is the command and the second element is the (optional) error. That groups them together both logically and visually.

tro3 · 2017-03-12T15:06:20Z

I agree that only the final command error need be checked.

sdboyer · 2017-03-12T15:08:05Z

OK, let's refactor that loop accordingly, then - it's an immediate failure if anything other than the final command reports an error.

domgreen · 2017-03-12T15:11:03Z

I agree, I think we should only expect a single error from the final command. Will do it in a min.

domgreen · 2017-03-12T16:25:30Z

Should now only look at the error on the final command, going to see if I can now use stderr instead of the error returned from the command.

…anifest exists

domgreen · 2017-03-12T17:45:48Z

My implementation may be a little naive so would appreciate feedback, I've basically done a strings.Contains to see if stderr contains the error we expect.

Also, simplified the error from init to help with testing, if this is a no no I will change back.

When running the test I do need to make sure a lock.json file, should probably fix this up as well.

tro3 · 2017-03-12T18:39:12Z

Down this path, we can't add auto update to the testcase.json file. I'm okay with this, subject to @sdboyer

domgreen · 2017-03-12T18:42:06Z

This last commit means we will not be forced to have lock or manifest in the final dir. Not sure if returning an empty string is the best thing to do here. But it does allow my manifest-exists test to run without having the redundant lock.json files.

tro3 · 2017-03-12T22:05:09Z

cmd/dep/integration_test.go

+				for i, args := range testCase.Commands {
+					err = testProj.DoRun(args)
+					if err != nil {
+						if i+1 != len(testCase.Commands) {


A sylistic nit, but I'd make this one statement if err != nil && i < len(testCase.commands)-1 to cut back on indentation lvls, and change the message to just "unexpected error raised: $s" (ie ditch the "non-final" bit).

Will do 😃 thanks

tro3 · 2017-03-12T22:05:26Z

cmd/dep/testdata/harness_tests/init/manifest-exists/final/manifest.json

@@ -0,0 +1 @@
+{}


What is this?

Empty manifest in final, thought it was needed if we pass the exception test will double check and remove if not needed.

I like the "don't check a nonexistent file" plan you proposed, in which case its not needed.

My question was more about the wierd symbol in the Files Changed area. Likely a "no ending newline" meaning.

Your right i think I can remove it when i get the non existing file issue sorted.

I thought it was probably best to keep this just to make sure that the manifest didn't change as it shouldn't have in this scenario.

tro3 · 2017-03-12T22:07:10Z

test/test.go

@@ -505,6 +505,10 @@ func (h *Helper) Path(name string) string {
 		joined = filepath.Join(h.tempdir, name)
 	}

+	if _, err := os.Stat(joined); os.IsNotExist(err) {
+		return ""
+	}


If this is to help with nonexistent final files, I think I'd move this to CompareFile. I'm ok changing the logic there to be "don't check file if not in final".

Yeah, so we can take out lock.json and poss (as above) the manifest.json if not needed in the final folder.
Reason I put it here (not that it feels right) is that we don't get into CompareFile as symlink check below fails.

We take another look in the morning at how I might be able to move this but recommendations welcome.

What do you mean by "symlink check fails"?

domgreen · 2017-03-12T22:13:54Z

test/test.go

+	if _, err := os.Stat(joined); os.IsNotExist(err) {
+		return ""
+	}
+
 	// Ensure it's the absolute, symlink-less path we're returning
 	abs, err := filepath.EvalSymlinks(joined)


@tro3 this line returns err if a file that it expects in final does not exist.

For which test case? When I remove final/manifest.json from a test and run it, I only get a proper "manifest created where none was expected" error from CompareFile. If this error was ignored, you'd get the effect you're looking for without the need for the above, no?

If i remove the check and run the manifest-exists test i get an error as before going into CompareFile it tries to get the path to the lock in
final.

Will run again and share the stack trace from the error in the morning.

Oh, I see - ProjPath (which calls TestHelper.Path) is getting called. I'd be comfortable rewriting ProjPath to be just what I had originally intended: $TMPDIR + ROOT + path, rather than running the symlink check. @sdboyer, does the symlink check called by TestHelper.Path really need to be done for these temp projects where all contents were copied over from testdata?

@sdboyer this is the initial reason i moved out the symlink check ... looking now if i can do it a different way 👍

domgreen · 2017-03-13T13:32:12Z

@sdboyer @tro3 - I have moved the check for the symlink stuff into the getFile method, this means we can keep it and also have the check for if the file exists. Think it makes more sense here. Thoughts?

domgreen · 2017-03-13T13:36:53Z

Not sure why Travis is unhappy, the error is:

�[31;1mThe command "test -z "$(gofmt -s -l . 2>&1 | grep -v vendor | tee /dev/stderr)"" exited with 1.�[0m

tro3 · 2017-03-14T00:24:15Z

Travis doesn't like something about the formatting in your integration_test.go mod. (Maybe too much indentation on line 58.) Run gomft on it.

domgreen · 2017-03-14T08:49:19Z

Thank you 👍 I am a total noob and relied on Atom to run fmt on save ... will know better for next time.

domgreen · 2017-03-14T20:15:44Z

@sdboyer @tro3 mind if i get a review? Will try and get any changes in this eve 😄

tro3 · 2017-03-15T02:04:52Z

I'm a bit of a noob myself, so I'll defer to @sdboyer for the professional review. My only comments:

Looks like you need to merge with master and fix the integration_test conflict.
I have a concern about the error parsing, in that the entire stderr is being grabbed. I might take a look later and see if there is a more specific way of parsing/extracting it, but it may not be possible.

sdboyer

good progress! mostly nits, but a couple things to fix.

sdboyer · 2017-03-15T01:47:15Z

cmd/dep/integration_test.go

+				for i, args := range testCase.Commands {
+					err = testProj.DoRun(args)
+					if err != nil && i < len(testCase.Commands)-1 {
+						t.Fatalf("unexpected error raised: %s", err.Error())


Let's include additional context here by also printing the name of the command that errored.

sdboyer · 2017-03-15T01:49:42Z

cmd/dep/testdata/harness_tests/README.md

-8. Check the `vendor` directory for the projects listed under `vendor-final`
-9. Check that there were no changes to `src` listings
-10. Clean up
+7. Check that only an expected error was raised from the final command


This wording here is a bit confusing. How about this:

"Ensure that, if any errors are raised, it is only by the final command and their string output matches error-expected"

sdboyer · 2017-03-15T01:52:02Z

test/integration_testcase.go

+	}
+
+	wantExists, want := len(tc.ErrorExpected) > 0, tc.ErrorExpected
+	gotExists, got := len(stderr) > 0, stderr


It's more idiomatic to compare to the empty string, rather than checking string length: tc.ErrorExpected != ""

sdboyer · 2017-03-15T02:09:44Z

test/integration_testcase.go

+		return false, "", nil
+	}
+
+	f, err := ioutil.ReadFile(abs)


Is this change actually needed? I believe ioutil.ReadFile() will traverse symlinks itself, if needed.

Or is it just so that we can differentiate the return values with the first parameter?

I realize this function isn't being introduced in this PR, but let's please add at least some named return values in order to clarify what's happening with those values: (exists bool, contents string, err error)

tbh, I am unsure if that symlink was needed so will remove it for the moment. As mentioned in the above comment I removed it from a different file as it caused the tests to fail when no lock.json was in the final dir.

sdboyer · 2017-03-15T02:10:04Z

test/integration_testcase.go

@@ -118,7 +140,14 @@ func getFile(path string) (bool, string, error) {
 	if err != nil {
 		return false, "", nil
 	}
-	f, err := ioutil.ReadFile(path)
+
+	// Ensure it's the absolute, symlink-less path we're returning


This comment does not seem to match with the actual goal of what's happening here - is it copied from elsewhere?

So I removed this from Path in test.go here as in my final I didnt have a lock.json (is some discussion above).

Will remove it from here as well for the moment.

domgreen · 2017-03-15T19:06:16Z

What is the opinion on passing stderr as a string should I look to pass the buffer instead?

tro3 · 2017-03-17T02:09:21Z

No strong opinion from me. I need to see the system in action first anyway, so my preference would be to merge it and tweak after that fact.

domgreen · 2017-03-18T12:10:37Z

@sdboyer have made suggested changes, I'm not sure why appveyor is stuck thought.

sdboyer

SO sorry for such a long delay 😢

Let's just encapsulate those symlink changes in a way that doesn't change any relevant conditions for other, non-harness tests, and then I think this is ready to go.

sdboyer · 2017-04-01T02:08:15Z

test/test.go

@@ -506,12 +506,7 @@ func (h *Helper) Path(name string) string {
 		joined = filepath.Join(h.tempdir, name)
 	}

-	// Ensure it's the absolute, symlink-less path we're returning
-	abs, err := filepath.EvalSymlinks(joined)


I'm not sure this is the best idea, as other tests with potentially different assumptions rely on it. If we really need to drop this for the harness' purposes, then could we introduce a separate method without the symlink call that the harness can use?

Yep no problem, am away in Flanders this weekend so will fix it up as soon as I'm home 👍

In the test TestIntegration/init/manifest-exists it doesnt like doing the SymLinks checkas the lock file doesnt exist in final and so the code I removes gives me this error:

lstat /private/var/folders/q9/jmtltdhs5hd8574dpdqgn40c0000gn/T/gotest220558389/src/github.com/golang/notexist/lock.json: no such file or directory

However, adding that file in /github.com/golang/dep/cmd/dep/testdata/harness_tests/init/manifest-exists/final seems abit counter intuative as when the tests ends it shouldnt be there as we are testing that if a manifest already exists for a reason we error before generating a lock file.

If I created a new method we would have to change the line https://github.com/golang/dep/blob/master/cmd/dep/integration_test.go#L60 so that it got to call the new method that handled the files not existing.

We could however just return an empty string if we got an error in from EvalSymlinks.

Would be good to have a bit of a shove in the right direction with how to choose to fix this one up.

domgreen · 2017-04-14T13:51:22Z

Will look at merging in master to this branch and see how its looking, or maybe just do a new PR whichever is easier.

@sdboyer any thoughts on the direction I should be going with the symlink issue?

sdboyer · 2017-04-16T22:06:50Z

@domgreen sorry for delay - honestly, it's because symlinks are kinda crushing my soul right now 😱

With how long this has been open and running, and if you're willing, it might be best to start a fresh PR rather than having all this history. Hopefully then I can cajole myself into diving down another symlink rabbit hole 😛

Again, sorry for the delay, and I appreciate your patience and continued effort on this so, so much!

domgreen · 2017-04-18T20:10:26Z

@sdboyer no probs, will do 😄

domgreen · 2017-04-19T19:47:55Z

replaced with #402

domgreen added 3 commits March 12, 2017 11:47

add ablity to test error scenarios

7630a71

removing init from update command

7c20d78

test case2 doesnt need init

48f87f5

googlebot added the cla: yes label Mar 12, 2017

domgreen mentioned this pull request Mar 12, 2017

add ablity to test error scenarios in test harness #317

Closed

sdboyer reviewed Mar 12, 2017

View reviewed changes

domgreen added 2 commits March 12, 2017 15:41

adding space

fb288d7

only able to expect errors on the final command

d85ee3e

checking if stderr contains error we expect, simplifying error when m…

2dfeafd

…anifest exists

no longer errors if final does not have lock or manifest

4563aa9

tro3 reviewed Mar 12, 2017

View reviewed changes

domgreen commented Mar 12, 2017

View reviewed changes

moving symlink check to getFile

aa5f279

im a noob

09a5179

carolynvs mentioned this pull request Mar 14, 2017

add a Makefile? #133

Closed

sdboyer suggested changes Mar 15, 2017

View reviewed changes

domgreen and others added 2 commits March 15, 2017 09:01

Merge branch 'master' into integration_test_errors_2

5def0c2

code review changes

7f0ba82

updating readme and outputting updated error

d8de93b

sdboyer suggested changes Apr 1, 2017

View reviewed changes

domgreen closed this Apr 19, 2017

test harness should be able to expect errors #319

test harness should be able to expect errors #319

Uh oh!

Conversation

domgreen commented Mar 12, 2017

Uh oh!

domgreen commented Mar 12, 2017

Uh oh!

domgreen commented Mar 12, 2017

Uh oh!

tro3 commented Mar 12, 2017 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdboyer commented Mar 12, 2017

Uh oh!

tro3 commented Mar 12, 2017

Uh oh!

sdboyer commented Mar 12, 2017

Uh oh!

domgreen commented Mar 12, 2017

Uh oh!

domgreen commented Mar 12, 2017

Uh oh!

domgreen commented Mar 12, 2017

Uh oh!

tro3 commented Mar 12, 2017

Uh oh!

domgreen commented Mar 12, 2017

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!

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!

domgreen commented Mar 13, 2017

Uh oh!

domgreen commented Mar 13, 2017

Uh oh!

tro3 commented Mar 14, 2017

Uh oh!

domgreen commented Mar 14, 2017

Uh oh!

domgreen commented Mar 14, 2017

Uh oh!

tro3 commented Mar 15, 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