Thanks for the review! It wasn't as scary as I had feared. > Looks like the Tag API tests suffered from doing unit-testing at an > integration-testing level. Good thing to see some tests lose the XML > parsing. The last of the API tests you edited looks like it might > benefit from self.expectThat(). I've switched to MatchesStructure.byEquality(). > > . > > In the docstring for populate_tags, can you say why the return value > is not meant to be used in production? Would there be any harm? The > caller doesn't want or need to block on it, of course, but that sort > of seems like the caller's problem. I've added the following: - You must not use the `Deferred` in the calling thread; it must only be manipulated in the reactor thread. Pretending it's not there is safer than chaining code onto it because it's easy to get wrong. - The call may not finish for 10 minutes or more. It is therefore not a good thing to be waiting for in a web request. > > . > > The docstring for _do_populate_tags confuses me no end. Did you mean > to rewrite it? There's also a typo in there: “oaram.” My mistake. It's a copy-n-paste from _get_clients_for_populating_tags (or something like that; there's some shared history in there). Fixed, I hope. > > More dangerous, of course, is the terminal preposition. Julian will > never forgive you. Done. > > . > > As you know I hate nontrivial nested callbacks with implicit closures. > Was there no way to use @inlineCallbacks in _do_populate_tags? I didn't know that. They're not nested though, and I wouldn't be able to have control over the returned Deferred if I used inlineCallbacks. I did try having a single nested function using inlineCallbacks, but it was no clearer than adding callbacks. It was worse if anything. Using callbacks with Deferreds gives a very clear control flow, and gives each function its own namespace, helping refactoring. > > . > > Why “list((uuid, name) for (uuid, name, _) in clusters)” and not > “[(uuid, name) for (uuid, name, _) in clusters]”? Just to avoid > variables leaking out of the comprehension? I hate that too, but it > doesn't seem worth our list comprehensions. To me, the diversity in > closing brackets, braces, and parentheses provides a valuable visual > cue of what the nesting levels are. Okay, I've changed it. Don't like it, but I've changed it ;) > > . > > Can you add a bug number to the XXX comment in > _get_clients_for_populating_tags? Yes! https://bugs.launchpad.net/maas/+bug/1372544 > > . > > test__returns_no_clients_when_there_is_an_error does not look right to > me. I thought _get_clients_for_populating_tags was supposed to return > the successfully obtained clients, and only left out clients for any > clusters that it couldn't get clients for. The tests don't cover the > difference between those two things AFAICS. Okay, done. > > . > > Shouldn't test__obtains_multiple_clients check somehow that it gets > clients _for the given clusters_, not just that it gets three clients? Done. > > . > > In TestDoPopulateTags.patch_clients, consider extracting a factory for > a fake client. Saves you having to loop over the clients and modify > them after you've already created the list. I've changed it to use create_autospec(Client, instance=True) to produce compatible mocks. > > . > > Could TestPopulateTags.patch_do_populate_tags use patch_autospec? Yes it could :) Done. > > . > > I find line breaks like these inconsistent with what we do elsewhere: > > self.assertThat(do_populate_tags, MockCalledOnceWith( > (), tag.name, tag.definition, populate_tags_module.tag_nsmap)) > > self.assertThat(do_populate_tags, MockCalledOnceWith( > clusters_expected, tag.name, tag.definition, > populate_tags_module.tag_nsmap)) > > for nodegroup, protocol in izip(nodegroups, protocols): > self.expectThat(protocol.EvaluateTag, MockCalledOnceWith( > protocol, tag_name=tag.name, tag_definition=tag.definition, > tag_nsmap=ANY, credentials=nodegroup.api_credentials)) > > If you're going to line-break the arguments list to self.assertThat > “inside one of the arguments,” I think that still makes it a > multi-line arguments list: > > self.assertThat( > do_populate_tags, > MockCalledOnceWith((), tag.name, tag.definition, > populate_tags_module.tag_nsmap)) > > self.assertThat( > do_populate_tags, > MockCalledOnceWith( > clusters_expected, tag.name, tag.definition, > populate_tags_module.tag_nsmap)) > > for nodegroup, protocol in izip(nodegroups, protocols): > self.expectThat( > protocol.EvaluateTag, > MockCalledOnceWith( > protocol, tag_name=tag.name, > tag_definition=tag.definition, > tag_nsmap=ANY, credentials=nodegroup.api_credentials)) > > Not that it's exactly pretty, but it gives an immediate overview of > the nesting and which part belongs at which level. It's useful when > you're scanning the code looking for things that you need to read more > closely. I've used this style for months. I find long tests (or any code) with lots of vertical space harder to read. My guess is that's it's something like: long scans/jumps up and down, missing the mark, finding my place again. I honestly cope better when I can keep the bulk of the test close together. If I find myself squinting at something I've written, I do spread things out, add blank lines, or comments more often. I do try not to obfuscate.