|
|
|
add PatchPath method
This consolidates a bunch of spots in tests that all just watch to patch the $PATH (and were doing so in a non-portable way).
https://code.launchpad.net/~natefinch/juju-core/035-pathlists/+merge/206984
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 6
Patch Set 2 : add PatchPath method #
Total messages: 4
|
natefinch
Please take a look.
|
11 years, 10 months ago (2014年02月18日 16:32:24 UTC) #1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Please take a look.
LGTM except that I'd suggest a slightly more meaningful name, say PatchExecPath, because there are many "paths" around the place, and it's perhaps not always obvious when Path refers to the shell's $PATH. One other suggestion below. https://codereview.appspot.com/65460043/diff/1/environs/sshstorage/storage_te... File environs/sshstorage/storage_test.go (right): https://codereview.appspot.com/65460043/diff/1/environs/sshstorage/storage_te... environs/sshstorage/storage_test.go:65: restoreEnv := testbase.PatchPath(s.bin) couldn't this just use s.PatchPath and lose the restoreEnv variable?
LGTM with a few suggestions. https://codereview.appspot.com/65460043/diff/1/testing/testbase/patch.go File testing/testbase/patch.go (right): https://codereview.appspot.com/65460043/diff/1/testing/testbase/patch.go#newc... testing/testbase/patch.go:63: // PatchPath provides a test a simple way to prepend path to the start of s/a test// s/prepend path/prepend a path/ https://codereview.appspot.com/65460043/diff/1/testing/testbase/patch.go#newc... testing/testbase/patch.go:64: // the environment's $PATH variable. A function is returned that will return the s/environment's $PATH variable/PATH environment variable/ ? s/A function is returned that will return/Returns a function that restores the/ https://codereview.appspot.com/65460043/diff/1/testing/testbase/patch.go#newc... testing/testbase/patch.go:66: func PatchPath(dir string) Restorer { s/PatchPath/PrependPath/ perhaps, because it more obviously describes what happens? https://codereview.appspot.com/65460043/diff/1/testing/testbase/patch_test.go File testing/testbase/patch_test.go (right): https://codereview.appspot.com/65460043/diff/1/testing/testbase/patch_test.go... testing/testbase/patch_test.go:89: d https://codereview.appspot.com/65460043/diff/1/testing/testbase/patch_test.go... testing/testbase/patch_test.go:93: expectPath := dir+string(os.PathListSeparator)+oldPath c.Check(os.Getenv("PATH"), gc.Equals, expectPath) ?