|
|
|
Fix 1307241. Isolate jujud.
Add the following mock functions to
environs/tools/testing/testing.go:
GetMockUploadTools
GetMockBundleTools
GetMockBuildTools
Make envtools.BundleTools mockable.
Patch functions in tests where
needed to ensure tests pass without
dependencies on jujud.
https://code.launchpad.net/~waigani/juju-core/1307241-isolate-from-jujud/+merge/215590
(do not edit description out of merge proposal)
Patch Set 1 #Patch Set 2 : Fix 1307241. Isolate jujud. #
Total comments: 2
Patch Set 3 : Fix 1307241. Isolate jujud. #
Total comments: 19
Patch Set 4 : Fix 1307241. Isolate jujud. #Patch Set 5 : Fix 1307241. Isolate jujud. #
Total comments: 30
Patch Set 6 : Fix 1307241. Isolate jujud. #
Total comments: 5
Total messages: 15
|
waigani
Please take a look.
|
11 years, 9 months ago (2014年04月15日 09:15:27 UTC) #1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Please take a look.
On 2014年04月15日 09:15:27, waigani wrote: > Please take a look. I'm not sure I understand the rational for this change. If jujud fails to compile, maybe that is where the test should stop.
On 2014年04月15日 10:10:15, dfc wrote: > On 2014年04月15日 09:15:27, waigani wrote: > > Please take a look. > > I'm not sure I understand the rational for this change. If jujud fails to > compile, maybe that is where the test should stop. We want to mock out the BundleTools because it adds unnecessary complexity to the tests which depend on it. eg when we test that tools are synced, we don't care that the real compile is not done; we just want a stub which runs quickly and is robust against whatever arch/series/os it is running on. Having said that, there need to be tests added which confirm that BundleTools itself works properly.
Looking good so far. I like that duped functionality has been consolidated. If we are mocking out BundleTools, we need tests for BundleTools itself to ensure that function does what it is supposed to do. We also need to ensure (if not done already) that the tests which use the mock assert that the mock is called correctly. https://codereview.appspot.com/87130045/diff/20001/cmd/juju/upgradejuju_test.go File cmd/juju/upgradejuju_test.go (right): https://codereview.appspot.com/87130045/diff/20001/cmd/juju/upgradejuju_test.... cmd/juju/upgradejuju_test.go:393: ttesting.ToolsDir = c.MkDir() Don't do this - make toolsDir an argument to GetMockBuildTools and pass in s.toolsDir https://codereview.appspot.com/87130045/diff/20001/environs/tools/testing/tes... File environs/tools/testing/testing.go (right): https://codereview.appspot.com/87130045/diff/20001/environs/tools/testing/tes... environs/tools/testing/testing.go:34: var ToolsDir string remove this and make it an arg to the functions which need it
Please take a look.
I don't think this is quite there yet. Definitely some good stuff, but we should chat. https://codereview.appspot.com/87130045/diff/40001/environs/bootstrap/bootstr... File environs/bootstrap/bootstrap_test.go (right): https://codereview.appspot.com/87130045/diff/40001/environs/bootstrap/bootstr... environs/bootstrap/bootstrap_test.go:24: ttesting "launchpad.net/juju-core/environs/tools/testing" explicit "toolstesting" throughout please 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... environs/sync/sync_test.go:275: os.Setenv("GOPATH", gopath) do we *have* to mess with the real environment? I accept that *perhaps* we do, but, eww. https://codereview.appspot.com/87130045/diff/40001/environs/sync/sync_test.go... environs/sync/sync_test.go:280: os.Setenv("GOPATH", os.Getenv("GOPATH")) 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 ;). https://codereview.appspot.com/87130045/diff/40001/environs/sync/sync_test.go... environs/sync/sync_test.go:440: // c.Assert(version{}, gc.IsNil) c.Assert(vers, gc.DeepEquals, version.Binary{}) ? https://codereview.appspot.com/87130045/diff/40001/environs/tools/build.go File environs/tools/build.go (right): https://codereview.appspot.com/87130045/diff/40001/environs/tools/build.go#ne... environs/tools/build.go:201: // BundleToolsFunc is a function which can bundles all the current juju tools s/bundles/bundle/ https://codereview.appspot.com/87130045/diff/40001/environs/tools/testing/tes... File environs/tools/testing/testing.go (right): https://codereview.appspot.com/87130045/diff/40001/environs/tools/testing/tes... environs/tools/testing/testing.go:34: // mockUploadTools simulates the effect of tools.Upload, but skips the time- GetMockUploadTools returns an UploadFunc that... https://codereview.appspot.com/87130045/diff/40001/environs/tools/testing/tes... environs/tools/testing/testing.go:37: // exposed and can itself be neatly mocked? 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. https://codereview.appspot.com/87130045/diff/40001/environs/tools/testing/tes... environs/tools/testing/testing.go:49: versions = append(versions, newVers) Don't we have a SeriesToUpload func somewhere? Shouldn't we be using that, rather copy/pasting its functionality? (or, better, mocking a slightly different layer such that the *only* thing we patch out is the costly build operation -- ISTM that all the rest of the time we really just want to patch out the building, because we can cheaply upload to dummy-provider storage anyway. Right?) https://codereview.appspot.com/87130045/diff/40001/environs/tools/testing/tes... environs/tools/testing/testing.go:62: toolsDir = filepath.Join(c.MkDir(), "jujud") 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? https://codereview.appspot.com/87130045/diff/40001/environs/tools/testing/tes... environs/tools/testing/testing.go:76: // getMockBuildTools returns a sync.BuildToolsTarballFunc implementation which generates s/getM/GetM/ https://codereview.appspot.com/87130045/diff/40001/environs/tools/testing/tes... environs/tools/testing/testing.go:82: stor, err := filestorage.NewFileStorageWriter(toolsDir) OK, I see what you're doing, but... this is so *not* about uploading anything. If there's useful functionality in UploadFakeToolsVersions, great; but please *extract* it and use the new method from both locations. This is just adding a layer of cruft, and makes me a sad panda. https://codereview.appspot.com/87130045/diff/40001/environs/tools/testing/tes... environs/tools/testing/testing.go:89: uploadedTools, err := testing.UploadFakeToolsVersions(stor, versions...) fwiw, the versions var STM to do nothing but obscure the fact that we're just uploading one version.
https://codereview.appspot.com/87130045/diff/40001/environs/bootstrap/bootstr... File environs/bootstrap/bootstrap_test.go (right): https://codereview.appspot.com/87130045/diff/40001/environs/bootstrap/bootstr... 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... 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... 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... 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/tes... File environs/tools/testing/testing.go (right): https://codereview.appspot.com/87130045/diff/40001/environs/tools/testing/tes... 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/tes... 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/tes... 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.
Please take a look.
On 2014年05月14日 02:49:37, waigani wrote: > Please take a look. Please see my comments here: https://codereview.appspot.com/87130045/#msg7
ISTM that we don't use GetMockUploadTools all *that* much, and it'd be worth dropping it (and just patching out the building/bundling, so we get fake tools put in the right place, in the right way). As it is I think GMUT is a liability, and it needs to be dropped either in this CL or a followup (but a followup *soon* -- I really don't want it hanging around). Please run this by wallyworld again, as well: he's much more au fait with the various tricky little details around here, but if he's happy I will be too (modulo my various comments here). https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go File environs/sync/sync_test.go (right): https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go... environs/sync/sync_test.go:262: type badBuildSuite struct { btw, please don't just insert this test suite in between the definition and the methods of the uploadSuite https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go... environs/sync/sync_test.go:273: // Create bad Go source file This isn't accurate any more https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go... environs/sync/sync_test.go:288: c.Assert(err, gc.IsNil) blank lines before comments in general, please https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go... environs/sync/sync_test.go:292: c.Assert(string(out), gc.Equals, "") I'm not sure you *really* need this bit, but equally you don't have to take it out :). The super-sweet spot would be to pull all the MakeTool-style functionality out into its own helpers and use all that same (tested) test-support code elsewhere -- but if you want to do that, it'd deserve its own CL. https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go... environs/sync/sync_test.go:317: // sync.bundleTools can be mocked to improve test speed. It's not 100% clear what this test is meant to be doing, indeed. Given what we have elsewhere I think I'd be testing this stuff by mocking the build/bundle stuff, and directly checking what gets uploaded. https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go... environs/sync/sync_test.go:452: c.Assert(err, gc.ErrorMatches, `build command "go" failed: exit status 1; `) please break all these cases into separate test methods where possible https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go... environs/sync/sync_test.go:480: func (s *uploadSuite) TestMockBundleTools(c *gc.C) { The SUT here is BuildToolsTarball, isn't it? We're testing that it calls BundleTools in the expected way. https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go... environs/sync/sync_test.go:483: writer io.Writer writer seems unused -- I'd expect us to write something to it and check that it was somehow reflected in the result of BuildToolsTarball... https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go... environs/sync/sync_test.go:493: sync.BuildToolsTarball(&version.Current.Number) error/result check? https://codereview.appspot.com/87130045/diff/80001/environs/tools/testing/tes... File environs/tools/testing/testing.go (right): https://codereview.appspot.com/87130045/diff/80001/environs/tools/testing/tes... environs/tools/testing/testing.go:52: } I'm still nervous that this is a parallel implementation of logic in uploadTools itself that *will* invitably change out of sync with this implementation. This is basically how mocking ends up going wrong IMO... https://codereview.appspot.com/87130045/diff/80001/environs/tools/testing/tes... environs/tools/testing/testing.go:69: not sure we need the blank lines here though :) https://codereview.appspot.com/87130045/diff/80001/environs/tools/testing/tes... environs/tools/testing/testing.go:90: stor, err := filestorage.NewFileStorageWriter(toolsDir) I still don't think we need to mess around with storage stuff. Can't we just write direct to a file in a directory? What do we get from the extra layer of complexity here? https://codereview.appspot.com/87130045/diff/80001/environs/tools/testing/tes... environs/tools/testing/testing.go:281: // UploadToStorage uploads tools and metadata for the specified versions to dir. FWIW this is UploadToDirectory
Please take a look. https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go File environs/sync/sync_test.go (right): https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go... environs/sync/sync_test.go:262: type badBuildSuite struct { On 2014年05月15日 08:57:20, fwereade wrote: > btw, please don't just insert this test suite in between the definition and the > methods of the uploadSuite Ah right. The suite goes before the methods for the suite. Got it. https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go... environs/sync/sync_test.go:273: // Create bad Go source file On 2014年05月15日 08:57:20, fwereade wrote: > This isn't accurate any more Done. https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go... environs/sync/sync_test.go:288: c.Assert(err, gc.IsNil) On 2014年05月15日 08:57:20, fwereade wrote: > blank lines before comments in general, please Done. https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go... environs/sync/sync_test.go:292: c.Assert(string(out), gc.Equals, "") On 2014年05月15日 08:57:19, fwereade wrote: > I'm not sure you *really* need this bit, but equally you don't have to take it > out :). > > The super-sweet spot would be to pull all the MakeTool-style functionality out > into its own helpers and use all that same (tested) test-support code elsewhere > -- but if you want to do that, it'd deserve its own CL. Do you mean a helper suite i.e. FakeToolsSuite or a helper package like utils? https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go... environs/sync/sync_test.go:317: // sync.bundleTools can be mocked to improve test speed. On 2014年05月15日 08:57:19, fwereade wrote: > It's not 100% clear what this test is meant to be doing, indeed. Given what we > have elsewhere I think I'd be testing this stuff by mocking the build/bundle > stuff, and directly checking what gets uploaded. Done. Is TestUploadToolsBadBuild enough? https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go... environs/sync/sync_test.go:452: c.Assert(err, gc.ErrorMatches, `build command "go" failed: exit status 1; `) On 2014年05月15日 08:57:19, fwereade wrote: > please break all these cases into separate test methods where possible Done. https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go... environs/sync/sync_test.go:480: func (s *uploadSuite) TestMockBundleTools(c *gc.C) { On 2014年05月15日 08:57:20, fwereade wrote: > The SUT here is BuildToolsTarball, isn't it? We're testing that it calls > BundleTools in the expected way. I don't follow sorry. I was testing if the mocked BundleTools gets called. I could have easily used sync.Upload instead. I also don't know what SUT means sorry. https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go... environs/sync/sync_test.go:483: writer io.Writer On 2014年05月15日 08:57:19, fwereade wrote: > writer seems unused -- I'd expect us to write something to it and check that it > was somehow reflected in the result of BuildToolsTarball... Done. https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go... environs/sync/sync_test.go:493: sync.BuildToolsTarball(&version.Current.Number) On 2014年05月15日 08:57:20, fwereade wrote: > error/result check? Done. https://codereview.appspot.com/87130045/diff/80001/environs/tools/testing/tes... File environs/tools/testing/testing.go (right): https://codereview.appspot.com/87130045/diff/80001/environs/tools/testing/tes... environs/tools/testing/testing.go:52: } On 2014年05月15日 08:57:20, fwereade wrote: > I'm still nervous that this is a parallel implementation of logic in uploadTools > itself that *will* invitably change out of sync with this implementation. This > is basically how mocking ends up going wrong IMO... Done. It's GONE GONE GONE! https://codereview.appspot.com/87130045/diff/80001/environs/tools/testing/tes... environs/tools/testing/testing.go:69: On 2014年05月15日 08:57:20, fwereade wrote: > not sure we need the blank lines here though :) Done. https://codereview.appspot.com/87130045/diff/80001/environs/tools/testing/tes... environs/tools/testing/testing.go:90: stor, err := filestorage.NewFileStorageWriter(toolsDir) On 2014年05月15日 08:57:20, fwereade wrote: > I still don't think we need to mess around with storage stuff. Can't we just > write direct to a file in a directory? What do we get from the extra layer of > complexity here? Done. https://codereview.appspot.com/87130045/diff/80001/environs/tools/testing/tes... environs/tools/testing/testing.go:281: // UploadToStorage uploads tools and metadata for the specified versions to dir. On 2014年05月15日 08:57:20, fwereade wrote: > FWIW this is UploadToDirectory Done. Did I do that?
LGTM
Thanks, this is great. LGTM with trivials. https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go File environs/sync/sync_test.go (right): https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go... environs/sync/sync_test.go:292: c.Assert(string(out), gc.Equals, "") On 2014年05月16日 03:06:07, waigani wrote: > On 2014年05月15日 08:57:19, fwereade wrote: > > I'm not sure you *really* need this bit, but equally you don't have to take it > > out :). > > > > The super-sweet spot would be to pull all the MakeTool-style functionality out > > into its own helpers and use all that same (tested) test-support code > elsewhere > > -- but if you want to do that, it'd deserve its own CL. > > Do you mean a helper suite i.e. FakeToolsSuite or a helper package like utils? I'd guess a suite would work well for easy integration of setup/teardown, but not necessarily. Whatever seems to work best :). https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go... environs/sync/sync_test.go:317: // sync.bundleTools can be mocked to improve test speed. On 2014年05月16日 03:06:08, waigani wrote: > On 2014年05月15日 08:57:19, fwereade wrote: > > It's not 100% clear what this test is meant to be doing, indeed. Given what we > > have elsewhere I think I'd be testing this stuff by mocking the build/bundle > > stuff, and directly checking what gets uploaded. > > Done. Is TestUploadToolsBadBuild enough? Yeah, as long as we exercise both the happy path and the sad one, I think we're good. https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go... environs/sync/sync_test.go:480: func (s *uploadSuite) TestMockBundleTools(c *gc.C) { On 2014年05月16日 03:06:08, waigani wrote: > On 2014年05月15日 08:57:20, fwereade wrote: > > The SUT here is BuildToolsTarball, isn't it? We're testing that it calls > > BundleTools in the expected way. > > I don't follow sorry. I was testing if the mocked BundleTools gets called. I > could have easily used sync.Upload instead. I also don't know what SUT means > sorry. System Under Test. And no need to apologise :). https://codereview.appspot.com/87130045/diff/80001/environs/tools/testing/tes... File environs/tools/testing/testing.go (right): https://codereview.appspot.com/87130045/diff/80001/environs/tools/testing/tes... environs/tools/testing/testing.go:52: } On 2014年05月16日 03:06:08, waigani wrote: > On 2014年05月15日 08:57:20, fwereade wrote: > > I'm still nervous that this is a parallel implementation of logic in > uploadTools > > itself that *will* invitably change out of sync with this implementation. This > > is basically how mocking ends up going wrong IMO... > > Done. It's GONE GONE GONE! w00t! https://codereview.appspot.com/87130045/diff/100001/environs/sync/sync_test.go File environs/sync/sync_test.go (right): https://codereview.appspot.com/87130045/diff/100001/environs/sync/sync_test.g... environs/sync/sync_test.go:284: // sync.bundleTools can be mocked to improve test speed. Hey, I think it's clicked. It's downloading the tools (and doing some basic checks on them) to check that they really got uploaded. I think I'd be happiest to see more direct, explicit tests: I think we actually have pretty detailed ones around bootstrap/upgrade-juju, but it'd be best to have more detail in the Upload tests and maybe a bit less in the cmd/juju ones. https://codereview.appspot.com/87130045/diff/100001/environs/sync/sync_test.g... environs/sync/sync_test.go:467: // Test that Upload func passes after BundleTools func is mocked out maybe put htis comment just above the preceding s.PatchValue? https://codereview.appspot.com/87130045/diff/100001/environs/sync/sync_test.g... environs/sync/sync_test.go:483: // mocked out similarly https://codereview.appspot.com/87130045/diff/100001/environs/tools/testing/te... File environs/tools/testing/testing.go (right): https://codereview.appspot.com/87130045/diff/100001/environs/tools/testing/te... environs/tools/testing/testing.go:58: // Write fake tools to storage Not really accurate any more https://codereview.appspot.com/87130045/diff/100001/environs/tools/testing/te... environs/tools/testing/testing.go:66: StorageName: tools.StorageName(vers), you can just use "name" here
On 2014年05月16日 08:29:36, fwereade wrote: > Thanks, this is great. LGTM with trivials. > https://codereview.appspot.com/87130045/diff/100001/environs/sync/sync_test.go > File environs/sync/sync_test.go (right): > > https://codereview.appspot.com/87130045/diff/100001/environs/sync/sync_test.g... > environs/sync/sync_test.go:284: // sync.bundleTools can be mocked to improve > test speed. > Hey, I think it's clicked. It's downloading the tools (and doing some basic > checks on them) to check that they really got uploaded. I think I'd be happiest > to see more direct, explicit tests: I think we actually have pretty detailed > ones around bootstrap/upgrade-juju, but it'd be best to have more detail in the > Upload tests and maybe a bit less in the cmd/juju ones. I don't want that in this CL, though, it's big enough already.