-
Notifications
You must be signed in to change notification settings - Fork 380
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clean up after Client tests + some minor stuff #387
Conversation
All of this looks pretty good, except I have concerns about the possible incompatibilities in how Windows and plan9(?) can potentially handle I will admit I haven’t looked too deeply into any of these specific cases, but we’ve have been bitten before with inconsistent application of |
All paths are sent to a locally running server, so the interpretation of path names should be the same. Also, TempFile uses filepath.Join internally, so you should already have noticed if there were any problem with it. |
Test failure seems unrelated:
on Go tip. |
The cause seems to be golang/go@b5ddc42. |
I think we should comment out that test with a link to the commit you linked to, and a |
Commented out with a TODO. Tests pass. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When checking errors, it’s highly recommended to use (require|assert).NoError()
and (require|assert).Error()
as they have more semantic context than IsNil
and IsNotNil
. The output from the assert.Nil/NotNil
is quite a bit less useful:
--- FAIL: TestFoo (0.00s)
foo_test.go:15:
Error Trace: foo_test.go:15
Error: Received unexpected error:
oh noes!
Test: TestFoo
foo_test.go:16:
Error Trace: foo_test.go:16
Error: Received unexpected error:
pkg/errors
github.com/puellanivis/test-notnil-vs-error.TestFoo
/home/snowgirl/Work/tmp/go/test-notnil-vs-error/foo_test.go:16
testing.tRunner
/usr/lib64/go/1.14/src/testing/testing.go:1054
runtime.goexit
/usr/lib64/go/1.14/src/runtime/asm_amd64.s:1373
Test: TestFoo
foo_test.go:17:
Error Trace: foo_test.go:17
Error: Expected nil, but got: &errors.errorString{s:"oh noes!"}
Test: TestFoo
foo_test.go:19:
Error Trace: foo_test.go:19
Error: An error is expected but got nil.
Test: TestFoo
foo_test.go:20:
Error Trace: foo_test.go:20
Error: Expected value not to be nil.
Test: TestFoo
@puellanivis Anything more to do here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pedantry: three tempfiles should probably be renamed to match the new pattern?
Done, and renamed the remaining twelve too. |
Looks good. 👍 :) |
I noticed that running the tests left a lot of clutter in $TMPDIR. This PR fixes that; if I run
I end up with an empty /tmp/sftp.
Also: