Code review comment for lp:~allenap/maas/rpc-tags-region

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.