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

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().

.

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.

.

The docstring for _do_populate_tags confuses me no end. Did you mean to rewrite it? There's also a typo in there: “oaram.”

More dangerous, of course, is the terminal preposition. Julian will never forgive you.

.

As you know I hate nontrivial nested callbacks with implicit closures. Was there no way to use @inlineCallbacks in _do_populate_tags?

.

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.

.

Can you add a bug number to the XXX comment in _get_clients_for_populating_tags?

.

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.

.

Shouldn't test__obtains_multiple_clients check somehow that it gets clients _for the given clusters_, not just that it gets three clients?

.

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.

.

Could TestPopulateTags.patch_do_populate_tags use patch_autospec?

.

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.