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

Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

I see no test to establish that NoReceivers actually works, nor that it only affects the signals you pass it. 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.

.

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.

.

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.

review: Approve

« Back to merge proposal