Code review comment for lp:~waigani/juju-core/1307241-isolate-from-jujud

https://codereview.appspot.com/87130045/diff/40001/environs/bootstrap/bootstrap_test.go
File environs/bootstrap/bootstrap_test.go (right):

https://codereview.appspot.com/87130045/diff/40001/environs/bootstrap/bootstrap_test.go#newcode24
environs/bootstrap/bootstrap_test.go:24: ttesting
"launchpad.net/juju-core/environs/tools/testing"
On 2014/05/07 12:52:05, fwereade wrote:
> explicit "toolstesting" throughout please

Done.

https://codereview.appspot.com/87130045/diff/40001/environs/sync/sync_test.go
File environs/sync/sync_test.go (right):

https://codereview.appspot.com/87130045/diff/40001/environs/sync/sync_test.go#newcode275
environs/sync/sync_test.go:275: os.Setenv("GOPATH", gopath)
On 2014/05/07 12:52:06, fwereade wrote:
> do we *have* to mess with the real environment? I accept that
*perhaps* we do,
> but, eww.

Done. Using s.PatchEnvPathPrepend.

https://codereview.appspot.com/87130045/diff/40001/environs/sync/sync_test.go#newcode280
environs/sync/sync_test.go:280: os.Setenv("GOPATH", os.Getenv("GOPATH"))
On 2014/05/07 12:52:05, fwereade wrote:
> This is wrong. You need to capture the result of Getenv at setup time
-- sadly,
> you can't just drop the defer and move the code and actually have it
work the
> same ;).

Done. I've mocked go cmd directly.

https://codereview.appspot.com/87130045/diff/40001/environs/sync/sync_test.go#newcode440
environs/sync/sync_test.go:440: // c.Assert(version{}, gc.IsNil)
On 2014/05/07 12:52:06, fwereade wrote:
> c.Assert(vers, gc.DeepEquals, version.Binary{}) ?

Done.

https://codereview.appspot.com/87130045/diff/40001/environs/tools/testing/testing.go
File environs/tools/testing/testing.go (right):

https://codereview.appspot.com/87130045/diff/40001/environs/tools/testing/testing.go#newcode37
environs/tools/testing/testing.go:37: // exposed and can itself be
neatly mocked?
On 2014/05/07 12:52:06, fwereade wrote:
> I feel like fixing *this* TODO might have been a step in a slightly
better
> direction, but I'm not totally sure at this point.

> edit: I'm getting surer. I'll be around later tonight, let's chat live
-- it's
> been a while since I saw this code, and I might be missing something,
but I
> think there's something funky about the layering here.

UploadTools was being mocked in several places in the tests before my
cl. Each place reproduced the same functionality, so I created this
function clean things up.

As such, shouldn't removing UploadTools be done in a separate branch? If
so, for now shouldn't we leave this function as it at least cleans up
all the stray mock UploadTools funcs and eases making UploadTools
unmockable?

What are you referring to when you talk about build logic in
agent/tools?

https://codereview.appspot.com/87130045/diff/40001/environs/tools/testing/testing.go#newcode62
environs/tools/testing/testing.go:62: toolsDir =
filepath.Join(c.MkDir(), "jujud")
On 2014/05/07 12:52:06, fwereade wrote:
> This seems strange to me... AFAICS the only client passes in "", and
the
> replacement "jujud" directory is always empty anyway, so can't we
just: drop the
> dir param, skip the Archive call, write nothing, return the hash of
the empty
> file, and be done?

Done.

https://codereview.appspot.com/87130045/diff/40001/environs/tools/testing/testing.go#newcode76
environs/tools/testing/testing.go:76: // getMockBuildTools returns a
sync.BuildToolsTarballFunc implementation which generates
On 2014/05/07 12:52:06, fwereade wrote:
> s/getM/GetM/

Done.

https://codereview.appspot.com/87130045/