Clean up after Client tests + some minor stuff

[greatroar]

I noticed that running the tests left a lot of clutter in $TMPDIR. This PR fixes that; if I run

mkdir /tmp/sftp
TMPDIR=/tmp/sftp go test -integration -testserver

I end up with an empty /tmp/sftp.

Also:

• Some tests were generating safe temporary paths with ioutil.TempFile, then assumed that a path derived from that (f.Name() + ".new" etc.) was also safe. These now use a directory, which also produces less clutter in $TMPDIR and means we only need one defer to clean up.
• Most tests generate files with more of the test name embedded in them, making missing cases easier to spot. This isn't necessary, but may serve as an example for future contributions.
• Tests that require uid = 0 now skip earlier.

[puellanivis]

All of this looks pretty good, except I have concerns about the possible incompatibilities in how Windows and plan9(?) can potentially handle filepath generated paths different from path.

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 path and filepath (former for remote paths, and later for local paths).

[greatroar]

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.

[greatroar]

Test failure seems unrelated:

=== RUN   TestMatch
    client_integration_test.go:1797: Match(`a[`, `a`) = false, "syntax error in pattern" want false, "<nil>"
--- FAIL: TestMatch (0.00s)

on Go tip.

[greatroar]

The cause seems to be golang/go@b5ddc42.

[puellanivis]

I think we should comment out that test with a link to the commit you linked to, and a TODO statement that go versions are going to disagree on this test all the way until (likely) go1.16 is our last supported version.

[greatroar]

Commented out with a TODO. Tests pass.

[puellanivis]

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

[greatroar]

@puellanivis Anything more to do here?

[puellanivis]

pedantry: three tempfiles should probably be renamed to match the new pattern?

[greatroar]

Done, and renamed the remaining twelve too.

[puellanivis]

Looks good. 👍 :)