Another example for bad testing

Today I discovered this in salt.

Someone tries to test the function iptables.get_saved_rules in salt/tests/unit/modules/iptables_test.py#L149

@patch('salt.modules.iptables._parse_conf', MagicMock(return_value=False))
def test_get_saved_rules(self):
    '''
    Test if it return a data structure of the rules in the conf file
    '''
    self.assertFalse(iptables.get_saved_rules())

But in fact acutally nothing is tested at all. Literally nothing.

So lets have a look at the corresponding code. salt/salt/modules/iptables.py#L471

def get_saved_rules(conf_file=None, family='ipv4'):
    '''
    Return a data structure of the rules in the conf file
    CLI Example:
    .. code-block:: bash
        salt '*' iptables.get_saved_rules
        IPv6:
        salt '*' iptables.get_saved_rules family=ipv6
    '''
    return _parse_conf(conf_file, family)

This function just forwards some parameters to another function _parse_conf. The above test mocks out exactly this call, but does not check anything about the mock. This test does not even cover deleting the only statment in this function! This is caused to self.assertFalse, which is not very strict and accepts None as False.

However, I discovered this code, since ipv6 is not working correctly, due to wrong parameter passing to _parse_conf. Having a look there, shows us the use of keywords-only.

def _parse_conf(conf_file=None, in_mem=False, family='ipv4'):
    '''
    If a file is not passed in, and the correct one for this OS is not
    detected, return False
    '''
    ...

So in case of get_saved_rules, ipv4 is passed as parameter in_mem instead of family. So the code always runs the ipv4-path.

Long story short. If you are mocking. Check if your expected call really happened. This code really tests the call for _parse_conf, with expected parameters:

def test_get_saved_rules(self):
    '''
    Test if it return a data structure of the rules in the conf file
    '''
    mock = MagicMock(return_value=False)
    with patch.object(iptables, '_parse_conf', mock):
        self.assertFalse(iptables.get_saved_rules())
        mock.assert_called_with(conf_file=None, family='ipv4')

Even this is not best practice, since calling stuff on mock objects, also silently irgnores errors. e.g. mock.fajdslksahgalds() would work totally fine, without any info. Most important in such cases. Write your test before the code, so you see the test failing once.

… Yes. I opened pull requests for both issues.