Code review comment for lp:~gmb/maas/add-alerts-for-disconnected-clusters-bug-1341121

On 3 October 2014 12:09, Jeroen T. Vermeulen <email address hidden> wrote:
> I'm sure this will be appreciated, even if it may be a little annoying to some people with distributed clusters.

Definitely! Thank you! Don't worry about being annoying.

Oh, wait, you mean the branch. Nevermind ;P

> Review notes inline.
>
> Diff comments:
>
> Tiny suggestion: instead of...
>
> any(
> not cluster.is_connected() for cluster in accepted_clusters)
>
> you could probably write...
>
> not all(
> cluster.is_connected() for cluster in accepted_clusters)

Yep, done.

>> +class ExternalComponentsMiddlewareTest(MAASServerTestCase):
>> + """Tests for the ExternalComponentsMiddleware."""
>> +
>> + def test__checks_connectivity_of_accepted_clusters(self):
>> + get_client_for = self.patch(nodegroup_module, 'getClientFor')
>> + middleware = ExternalComponentsMiddleware()
>> + clusters = {
>> + factory.make_NodeGroup(status=NODEGROUP_STATUS.ACCEPTED)
>> + for _ in range(3)}
>
> Why are we creating 3 of everything? For luck? If the code is correct for 2, it's probably correct for 3. (And in Python, if it's correct for 1 chances are it's correct for 2. It's not like C where you might do *ptr instead of ptr[i] for every i.)
>
> Also, we've taken to putting the closing brace/bracket of a comprehension on a line of its own.

You know, I honestly don't know why. Habit, I guess. Three is a magic
number. It's not really relevant to have > 1 cluster in this test
anyway, I'll change it.

>> +
>> + request = factory.make_fake_request(factory.make_string(), 'GET')
>> + middleware.process_request(request)
>> +
>> + self.assertThat(
>> + get_client_for, MockCallsMatch(
>> + *[call(cluster.uuid, timeout=0) for cluster in clusters]))
>> +
>> + def test__ignores_non_accepted_clusters(self):
>> + get_client_for = self.patch(nodegroup_module, 'getClientFor')
>> + middleware = ExternalComponentsMiddleware()
>> + statuses = (
>> + NODEGROUP_STATUS.PENDING,
>> + NODEGROUP_STATUS.ACCEPTED,
>> + NODEGROUP_STATUS.REJECTED)
>> + clusters = {
>> + factory.make_NodeGroup(status=random.choice(statuses))
>> + for _ in range(3)}
>
> Again with the three! But why bother testing three random items from a list of 3 values, when it's actually easier to test all values?
>
> Although in this case I personally would do it differently: write one simple test just for ACCEPTED, and one simple test for a random other status. In the event that we get it wrong for pending clusters but not rejected ones, or vice versa, that leaves a fifty-fifty chance of the test passing by accident on any given run, so up to you to judge how likely that is.

Right.

> Also, there's no need to enumerate the statuses manually; just use factory.pick_enum(NODEGROUP_STATUS, but_not=[NODEGROUP_STATUS.ACCEPTED]).

Ah, didn't know about that, thanks.

>> + accepted_cluster_uuids = (
>> + cluster.uuid for cluster in clusters
>> + if cluster.status == NODEGROUP_STATUS.ACCEPTED)
>> +
>> + request = factory.make_fake_request(factory.make_string(), 'GET')
>> + middleware.process_request(request)
>> +
>> + self.assertThat(
>> + get_client_for, MockCallsMatch(
>> + *[call(uuid, timeout=0) for uuid in accepted_cluster_uuids]))
>> +
>> + def test_registers_error_if_there_are_disconnected_clusters(self):
>
> This may just be bikeshedding, but I don't see any tests to test the difference between “there are disconnected clusters” and “all clusters are disconnected.” That could actually matter if somebody got the negation and the any() wrong.
>
> (Not naming names but at least one person in this company hates the De Morgan transformations. As you can see I like them. But they're not foolproof.).
>
> Worth a separate test maybe?

Yes.