Code review comment for lp:~jason-hobbs/maas/lp-1210393

Looks good, thanks again!

[1]

+    def test_runs_bmc_config(self):
+        """Ensure bmc-config is called with the correct args."""
+        recorder = self.patch(maas_ipmi_autodetect, 'run_command')
+
+        section = 'foo'
+        bmc_get_section(section)
+        args = recorder.call_args[0][0]
+
+        self.assertEqual('bmc-config', args[0])
+
+        for arg in ['--checkout', '--section', section]:
+            self.assertIn(arg, args)

The ordering is static so you can save some pain by using:

        recorder.assert_called_once_with(
            'bmc-config', '--checkout', '--section', section)

It'll also give a more useful error message if it fails.

I think there's also an assert_called_with() that matches any of
multiple calls, but if you need it you ought to check.

[2]

+        bgs_mock = Mock(return_value=user_text)
+        self.patch(maas_ipmi_autodetect, 'bmc_get_section', bgs_mock)
+        ps_mock = Mock(return_value=(user_number, user_record))
+        self.patch(maas_ipmi_autodetect, 'parse_section', ps_mock)

self.patch() does some of the above for you. You could also write it as:

        bgs_mock = self.patch(maas_ipmi_autodetect, 'bmc_get_section')
        bgs_mock.return_value = user_text
        ps_mock = self.patch(maas_ipmi_autodetect, 'parse_section')
        ps_mock.return_value = (user_number, user_record)

It only saves the import of Mock, but you might like it.

[3]

+        self.assertEqual(user_number, bgs_mock.call_args[0][0])
+        self.assertEqual(user_text, ps_mock.call_args[0][0])

Again, assert_called_once_with() is your friend.

If you need to match something with multiple arguments, but you can only
about one or two, there's ANY:

        from mock import ANY
        thing.assert_called_once_with("important", ANY, keyword=ANY)

There's also call(), which also works with ANY:

        from mock import call
        self.assertEqual(
            [call(1, ANY), call(2, ANY)],
            thing.call_args_list)

[4]

+def disabled_user(update=None):
+    """Record for a disabled user."""
+    base = {'Enable_User': 'No'}
+
+    if update:
+        base.update(update)
+
+    return base

Can you name this make_disabled_user()? All test factories have the
prefix "make", except where they don't, er, they have the prefix "get".

[5]

+def base_attributes(attributes_update=None):
+    """Base user records with updates in an OrderedDict."""
+
+    attributes_template = {
+        """Starting point for user records."""
+        'User1': {
+            'Enable_User': 'Yes'
+        },
+        'User2': {
+            'Username': 'admin',
+            'Enable_User': 'Yes'
+        }
+    }
+
+    base = OrderedDict(deepcopy(attributes_template))

I don't think the deepcopy() is necessary.

[6]

+    if attributes_update:
+        base.update(attributes_update)

Another habit we have is never relying on the implicit boolean-ness of
things in Python. Explicitly test for None instead:

    if attributes_update is not None:
        base.update(attributes_update)

[7]

+        ('one_disabled_not_blank_user', dict(

Fwiw, scenario names don't have to be valid identifiers; they're
free-form text I think.