nagios-charmers team mailing list archive

[Stuart Bishop <stuart.bishop@xxxxxxxxxxxxx>]

Review: Needs Fixing

All looks good, apart from the use of os.getcwd() in tests per inline comments.

Diff comments:

> diff --git a/src/tests/unit/test_cron_mdadm.py b/src/tests/unit/test_cron_mdadm.py
> index d124931..75d291c 100644
> --- a/src/tests/unit/test_cron_mdadm.py
> +++ b/src/tests/unit/test_cron_mdadm.py
> @@ -100,3 +100,35 @@ class TestCronMdadm(unittest.TestCase):
>                      ' /dev/md2 ok')
>          self.assertTrue(cron_mdadm.parse_output())
>          genout.assert_called_once_with(expected)
> +
> +    @mock.patch('cron_mdadm.generate_output')
> +    @mock.patch('cron_mdadm.get_devices')
> +    @mock.patch('subprocess.check_output')
> +    def test_parse_output_removed(self, mdadm_details, devices, genout):
> +        test_output = os.path.join(
> +            os.getcwd(), 'tests', 'hw-health-samples',

Better to define a module level constant like TESTS_DIR = os.path.join(os.path.basename(__FILE__), 'tests') and use it, rather than assuming that os.getcwd() returns what you expect.

> +            'mdadm.output.critical.2')
> +        with open(test_output, 'r') as fd:
> +            mdadm_details.return_value = ''.join(fd.readlines()).encode()
> +
> +        devices.return_value = set(['/dev/md1'])
> +        genout.return_value = True
> +        expected = ('CRITICAL: /dev/md1 degraded, RaidDevice(s) 2 marked removed')
> +        self.assertTrue(cron_mdadm.parse_output())
> +        genout.assert_called_once_with(expected)
> +
> +    @mock.patch('cron_mdadm.generate_output')
> +    @mock.patch('cron_mdadm.get_devices')
> +    @mock.patch('subprocess.check_output')
> +    def test_parse_output_rebuilding(self, mdadm_details, devices, genout):
> +        test_output = os.path.join(
> +            os.getcwd(), 'tests', 'hw-health-samples',

Here too... avoid assuming the current working directory. It makes it a pain to rework makefiles and switching test runners etc.

> +            'mdadm.output.warning')
> +        with open(test_output, 'r') as fd:
> +            mdadm_details.return_value = ''.join(fd.readlines()).encode()
> +
> +        devices.return_value = set(['/dev/md0', '/dev/md1', '/dev/md2'])
> +        genout.return_value = True
> +        expected = ('WARNING: /dev/md1 recovering, /dev/sdk2 rebuilding (13% complete)')
> +        self.assertTrue(cron_mdadm.parse_output())
> +        genout.assert_called_once_with(expected)


-- 
https://code.launchpad.net/~afreiberger/hw-health-charm/+git/hw-health-charm/+merge/374838
Your team Nagios Charm developers is requested to review the proposed merge of ~afreiberger/hw-health-charm:add-mdadm-checks into hw-health-charm:master.