MAAS

Code review comment for lp:~julian-edwards/maas/stop-dhcp-server-bug-1283114

stop-dhcp-server-bug-1283114
Merge into trunk

Revision history for this message

Julian Edwards (julian-edwards) wrote on 2014-02-25:

On 25/02/14 16:35, Jeroen T. Vermeulen wrote:
> Review: Approve

Thanks for the review.

> I see no test to establish that NoReceivers actually works,

What?! I added one, it's in the diff?:

+class TestNoReceivers(MAASServerTestCase):

If you are implying that I should test that no signal receivers are
actually called - then you'd be asking for a test that tests Django
signals functionality.

> nor that it only affects the signals you pass it.

That would be impossible unless there was a registry of all known
signals, which AFAIK there isn't. They can be created at any time.

> Given the earlier confusion with the global signals list, neither is a given. Landing this code both unused and untested seems irresponsible, whereas putting more work into it when we don't really know that we'll be needing it seems pointless. So if I were you I'd just drop it. If we do ever need it, searching the internet for NoReceivers will bring us back to this MP.

I think that's a rather harsh assessment. IIRC Raphaël made a good case
for it after I wanted to drop it but I can't remember what he said now.
Perhaps he'll chime in.

(I think it was something to do with speeding up the test suite, because
we have huge numbers of useless signals firing in every test that sets
up dhcp/dns related objects.)

> The simplification of test_report_foreign_dhcp_does_not_trigger_update_signal is great. One small thing though:
>
> 303 - self.assertEqual([], patch.mock_calls)
> 304 + self.assertEqual(0, tasks.write_dhcp_config.apply_async.call_count)
>
> Testing against [] is usually more helpful than testing against a call_count of zero, because the test's failure will tell you what calls were made, which can help in debugging.

You're right - I originally tried to do that and got caught up with
mock's innards. It doesn't have an assert_not_called() function :(

It turns out that this works:
tasks.write_dhcp_config.apply_async.assert_has_calls([])

>
> .
>
> Twice in src/provisioningserver/tests/test_tasks.py, as well as once in test_write_dhcp_config_called_when_no_managed_interfaces, you use Mock.assert_called_once_with. Please avoid this method — it's prone to false negatives due to a pitfall in Mock's design. If there is ever a typo in the assertion method's name, the Mock will see it as a regular method call, and very helpfully mock it. The result: an assertion that can't fail.
>
> Use an equality check on Mock.mock_calls instead. It's harder to lose a typo in "mock_calls," and when we do, it will lead to a false positive which will immediately compel us to fix the typo.
>

Can I leave them here pretty please? If I promise to write a testtools
matcher so this can't happen again....

On 25/02/14 16:35, Jeroen T. Vermeulen wrote:
> Review: Approve

Thanks for the review.

> I see no test to establish that NoReceivers actually works,

What?!  I added one, it's in the diff?:

+class TestNoReceivers(MAASServerTestCase):

If you are implying that I should test that no signal receivers are
actually called - then you'd be asking for a test that tests Django
signals functionality.

> nor that it only affects the signals you pass it.

That would be impossible unless there was a registry of all known
signals, which AFAIK there isn't.  They can be created at any time.

>  Given the earlier confusion with the global signals list, neither is a given.  Landing this code both unused and untested seems irresponsible, whereas putting more work into it when we don't really know that we'll be needing it seems pointless.  So if I were you I'd just drop it.  If we do ever need it, searching the internet for NoReceivers will bring us back to this MP.

I think that's a rather harsh assessment.  IIRC Raphaël made a good case
for it after I wanted to drop it but I can't remember what he said now.
 Perhaps he'll chime in.

(I think it was something to do with speeding up the test suite, because
we have huge numbers of useless signals firing in every test that sets
up dhcp/dns related objects.)

> The simplification of test_report_foreign_dhcp_does_not_trigger_update_signal is great.  One small thing though:
> 
> 303	-        self.assertEqual([], patch.mock_calls)
> 304	+        self.assertEqual(0, tasks.write_dhcp_config.apply_async.call_count)
> 
> Testing against [] is usually more helpful than testing against a call_count of zero, because the test's failure will tell you what calls were made, which can help in debugging.

You're right - I originally tried to do that and got caught up with
mock's innards.  It doesn't have an assert_not_called() function :(

It turns out that this works:
    tasks.write_dhcp_config.apply_async.assert_has_calls([])

> 
> .
> 
> Twice in src/provisioningserver/tests/test_tasks.py, as well as once in test_write_dhcp_config_called_when_no_managed_interfaces, you use Mock.assert_called_once_with.  Please avoid this method — it's prone to false negatives due to a pitfall in Mock's design.  If there is ever a typo in the assertion method's name, the Mock will see it as a regular method call, and very helpfully mock it.  The result: an assertion that can't fail.
> 
> Use an equality check on Mock.mock_calls instead.  It's harder to lose a typo in "mock_calls," and when we do, it will lead to a false positive which will immediately compel us to fix the typo.
>

Can I leave them here pretty please?  If I promise to write a testtools
matcher so this can't happen again....

« Back to merge proposal