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.