[Maas-devel] Be careful when you use Mock patching in tests

[Gavin Panella]

On 5 November 2013 06:55, Julian Edwards <julian.edwards at canonical.com> wrote:
> I just had to make this change because amazingly it had two innocuous bugs:
>
> -------
>   def test_write_full_dns_doesnt_call_task_it_no_interface_configured(self):
>          self.patch(settings, 'DNS_CONNECT', True)
> -        patched_task = self.patch(tasks, 'write_full_dns_config')
> +        patched_task = self.patch(dns.tasks.write_full_dns_config, "delay")
>          dns.write_full_dns_config()
>          self.assertEqual(0, patched_task.call_count)
> -------
>
> Bug #1: Was naively patching the global "tasks" when it should be
> patching the one that the dns module imported.

The dns.tasks module is the same as the tasks module imported here, so
the effect is the same.

>
> Bug #2: When patching out celery jobs, you need to patch the delay() call.

This test would have been valid like this too:

  def test_write_full_dns_doesnt_call_task_it_no_interface_configured(self):
         self.patch(settings, 'DNS_CONNECT', True)
         patched_task = self.patch(tasks, 'write_full_dns_config')
         dns.write_full_dns_config()
-        self.assertEqual(0, patched_task.call_count)
+        self.assertEqual(0, patched_task.delay.call_count)

or, better:

-        self.assertEqual(0, patched_task.call_count)
+        self.assertEqual([], patched_task.delay.mock_calls)

I'm not dismissing what you're saying though: whichever way you look at
this, getting the test to fail first would have prevented these bugs
from getting in.