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

[Julian Edwards]

On 05/11/13 20:16, Gavin Panella wrote:
> 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.

Huh - I've seen this screw up before, but you're right.  I'm trying to
remember why I've had to do this kind of thing before ...

> 
>>
>> 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.
> 

Which was my point.