Base utils tests on IsolationSuite.

Base utils tests on IsolationSuite.

This is mostly a mechanical branch.

Add package_test files where missing.
Fix unregistered utilsTest suite (tests not being executed).
Remove dependency tests: they will be replaced by a global 
test after the migration.

At this point, all packages in utils are self-contained,
except for the following:
- syslog: uses agent.DefaultLogDir;
- zip: uses testing/filetesting 
  (not a problem since filetesting will be moved to github testing);
- ssh: uses cmd RcPassthroughError stuff and testing.FakeHomeSuite.

https://code.launchpad.net/~frankban/juju-core/introduce-isolationsuite/+merge/221377

(do not edit description out of merge proposal)

[frankban, 2014-05-29 13:24:22]

Please take a look.

[frankban, 2014-05-29 13:37:39]

Please take a look.

[rog, 2014-05-29 15:25:44]

LGTM modulo a few comments below.

https://codereview.appspot.com/92700044/diff/20001/utils/file_test.go
File utils/file_test.go (left):

https://codereview.appspot.com/92700044/diff/20001/utils/file_test.go#oldcode178
utils/file_test.go:178: data, err = ioutil.ReadFile(path)
weird, how did this work before?

https://codereview.appspot.com/92700044/diff/20001/utils/file_test.go
File utils/file_test.go (right):

https://codereview.appspot.com/92700044/diff/20001/utils/file_test.go#newcode28
utils/file_test.go:28: err := utils.SetHome("/home/test-user")
I'd prefer it if this was set only for TestNormalizePath, which seems to be the
only test that requires it. I don't want to see something inadvertently trying
to write to the real /home/test-user, which may actually exist.

https://codereview.appspot.com/92700044/diff/20001/utils/fslock/fslock_test.go
File utils/fslock/fslock_test.go (right):

https://codereview.appspot.com/92700044/diff/20001/utils/fslock/fslock_test.g...
utils/fslock/fslock_test.go:136: case <-time.After(50 * time.Millisecond):
as mentioned online , I think this would be better with locally defined
constants shortWait and longWait.

https://codereview.appspot.com/92700044/diff/20001/utils/set/strings_test.go
File utils/set/strings_test.go (right):

https://codereview.appspot.com/92700044/diff/20001/utils/set/strings_test.go#...
utils/set/strings_test.go:16: testing.IsolationSuite
honestly, i think that IsolationSuite is probably overkill for this test suite -
it's just manipulating data.

https://codereview.appspot.com/92700044/diff/20001/utils/ssh/ssh_test.go
File utils/ssh/ssh_test.go (right):

https://codereview.appspot.com/92700044/diff/20001/utils/ssh/ssh_test.go#newc...
utils/ssh/ssh_test.go:33: s.originalPath = os.Getenv("PATH")
We probably want to restore the original path in SSHCommandSuite.TearDownSuite.

I wonder if it might make sense to add an "ExcludeEnvVars" field to
testing.IsolationSuite that could be set at the start of SetUpSuite
and then would exclude the chosen env vars from isolation.

[fwereade, 2014-05-29 21:26:56]

Just adding to rog's comments.

Re the "data" var in utils_test... *this* sort of thing is why we don't put test
data (or anything...) in globals unless we *have* to. That's the second case of
wanton global abuse I've seen in our tests, and it seems foolhrady to imagine
that we've just purged the last one.

But at least this one was accidental... the other one was entirely cold-blooded
and deliberate :/.

https://codereview.appspot.com/92700044/diff/20001/utils/file_test.go
File utils/file_test.go (left):

https://codereview.appspot.com/92700044/diff/20001/utils/file_test.go#oldcode178
utils/file_test.go:178: data, err = ioutil.ReadFile(path)
On 2014/05/29 15:25:43, rog wrote:
> weird, how did this work before?

trivial_test.go:28

/me shudders

https://codereview.appspot.com/92700044/diff/20001/utils/file_test.go
File utils/file_test.go (right):

https://codereview.appspot.com/92700044/diff/20001/utils/file_test.go#newcode28
utils/file_test.go:28: err := utils.SetHome("/home/test-user")
On 2014/05/29 15:25:43, rog wrote:
> I'd prefer it if this was set only for TestNormalizePath, which seems to be
the
> only test that requires it. I don't want to see something inadvertently trying
> to write to the real /home/test-user, which may actually exist.

+1 to using SetHome only for the tests that need it, but I don't see any reason
to use "/home/test-user" anyway when we could use c.MkDir() and explicitly join
to that path for the "~" tests.

https://codereview.appspot.com/92700044/diff/20001/utils/fslock/fslock_test.go
File utils/fslock/fslock_test.go (right):

https://codereview.appspot.com/92700044/diff/20001/utils/fslock/fslock_test.g...
utils/fslock/fslock_test.go:136: case <-time.After(50 * time.Millisecond):
On 2014/05/29 15:25:43, rog wrote:
> as mentioned online , I think this would be better with locally defined
> constants shortWait and longWait.

+1

https://codereview.appspot.com/92700044/diff/20001/utils/set/strings_test.go
File utils/set/strings_test.go (right):

https://codereview.appspot.com/92700044/diff/20001/utils/set/strings_test.go#...
utils/set/strings_test.go:16: testing.IsolationSuite
On 2014/05/29 15:25:43, rog wrote:
> honestly, i think that IsolationSuite is probably overkill for this test suite
-
> it's just manipulating data.

Think of it as a seatbelt. It might not be strictly necessary if you're just
moving your car from the driveway into the garage, but you don't analyse the
cost/benefit every time: you just wear the damn thing because it's a good habit
to be in.

https://codereview.appspot.com/92700044/diff/20001/utils/ssh/ssh_test.go
File utils/ssh/ssh_test.go (right):

https://codereview.appspot.com/92700044/diff/20001/utils/ssh/ssh_test.go#newc...
utils/ssh/ssh_test.go:33: s.originalPath = os.Getenv("PATH")
On 2014/05/29 15:25:44, rog wrote:
> We probably want to restore the original path in
SSHCommandSuite.TearDownSuite.

Won't IsolationSuite do that for us? But, yeah, it would be good practice to add
a TearDownTest to reverse what we do in SetUpTest here.

> I wonder if it might make sense to add an "ExcludeEnvVars" field to
> testing.IsolationSuite that could be set at the start of SetUpSuite
> and then would exclude the chosen env vars from isolation.

Hmm, would you look into this a bit? The fake commands we set up below make me
think we shouldn't need the original $PATH at all. If we do, please figure out
why, and if we can avoid it.

And, please, let's not add DontActuallyIsolateLol methods to IsolationSuite.

[frankban, 2014-05-30 09:17:13]

Please take a look.

https://codereview.appspot.com/92700044/diff/20001/utils/file_test.go
File utils/file_test.go (left):

https://codereview.appspot.com/92700044/diff/20001/utils/file_test.go#oldcode178
utils/file_test.go:178: data, err = ioutil.ReadFile(path)
On 2014/05/29 21:26:55, fwereade wrote:
> On 2014/05/29 15:25:43, rog wrote:
> > weird, how did this work before?
> 
> trivial_test.go:28
> 
> /me shudders

Yeah, as I mentioned on IRC, data was already defined globally in another test
file of the same package. This also caused the other test to fail due to the
data value being changed here. This was hidden by the fact the test was not
executed (wrong suite registration).

https://codereview.appspot.com/92700044/diff/20001/utils/file_test.go
File utils/file_test.go (right):

https://codereview.appspot.com/92700044/diff/20001/utils/file_test.go#newcode28
utils/file_test.go:28: err := utils.SetHome("/home/test-user")
On 2014/05/29 21:26:55, fwereade wrote:
> On 2014/05/29 15:25:43, rog wrote:
> > I'd prefer it if this was set only for TestNormalizePath, which seems to be
> the
> > only test that requires it. I don't want to see something inadvertently
trying
> > to write to the real /home/test-user, which may actually exist.
> 
> +1 to using SetHome only for the tests that need it, but I don't see any
reason
> to use "/home/test-user" anyway when we could use c.MkDir() and explicitly
join
> to that path for the "~" tests.

Done.

https://codereview.appspot.com/92700044/diff/20001/utils/fslock/fslock_test.go
File utils/fslock/fslock_test.go (right):

https://codereview.appspot.com/92700044/diff/20001/utils/fslock/fslock_test.g...
utils/fslock/fslock_test.go:136: case <-time.After(50 * time.Millisecond):
On 2014/05/29 15:25:43, rog wrote:
> as mentioned online , I think this would be better with locally defined
> constants shortWait and longWait.

Done.

https://codereview.appspot.com/92700044/diff/20001/utils/ssh/ssh_test.go
File utils/ssh/ssh_test.go (right):

https://codereview.appspot.com/92700044/diff/20001/utils/ssh/ssh_test.go#newc...
utils/ssh/ssh_test.go:33: s.originalPath = os.Getenv("PATH")
On 2014/05/29 21:26:56, fwereade wrote:
> On 2014/05/29 15:25:44, rog wrote:
> > We probably want to restore the original path in
> SSHCommandSuite.TearDownSuite.
> 
> Won't IsolationSuite do that for us? But, yeah, it would be good practice to
add
> a TearDownTest to reverse what we do in SetUpTest here.
> 
> > I wonder if it might make sense to add an "ExcludeEnvVars" field to
> > testing.IsolationSuite that could be set at the start of SetUpSuite
> > and then would exclude the chosen env vars from isolation.
> 
> Hmm, would you look into this a bit? The fake commands we set up below make me
> think we shouldn't need the original $PATH at all. If we do, please figure out
> why, and if we can avoid it.

Done. Now we don't set the PATH.

> 
> And, please, let's not add DontActuallyIsolateLol methods to IsolationSuite.

[fwereade, 2014-05-30 09:27:41]

LGTM despite my concerns about file_test: progress not perfection. But please
add a TODO and a bug# to point out that this is totally lame and we ought to add
an indirection layer (possibly by adding user stuff to osenv, and mandating that
people use that?).

https://codereview.appspot.com/92700044/diff/40001/utils/file_test.go
File utils/file_test.go (right):

https://codereview.appspot.com/92700044/diff/40001/utils/file_test.go#newcode30
utils/file_test.go:30: currentUser, err := user.Current()
Hmm, IMO this is an isolation problem too.

https://codereview.appspot.com/92700044/diff/40001/utils/file_test.go#newcode59
utils/file_test.go:59: err:  "user: unknown user foobar",
What if there's a foobar user?

[frankban, 2014-05-30 09:33:54]

Please take a look.

[frankban, 2014-05-30 09:45:17]

Please take a look.

[frankban, 2014-05-30 09:46:04]

On 2014/05/30 09:27:41, fwereade wrote:
> LGTM despite my concerns about file_test: progress not perfection. But please
> add a TODO and a bug# to point out that this is totally lame and we ought to
add
> an indirection layer (possibly by adding user stuff to osenv, and mandating
that
> people use that?).

Done.