Improved tests for host mod and create commands.

This is preparation for a refactor of the logic for those commands. For
host mod removed the --ignore_site_file flag to ensure real logic is
always tested. For host create replaced the existing tests with tests
that cover the site logic including all command line arguments.

Remaining changes are to satisfy linter. Any comments on the docstrings
will be welcome. Much of it was inferred from calls I could find in the
codebase.

BUG=None
TEST=Yes, lots of them

Change-Id: Ib882c54318bca0f29f48de0da8c8f2df5e090a0c
Reviewed-on: https://chromium-review.googlesource.com/354552
Commit-Ready: Justin Giorgi <jgiorgi@google.com>
Commit-Ready: Justin Giorgi <jgiorgi@chromium.org>
Tested-by: Justin Giorgi <jgiorgi@google.com>
Tested-by: Justin Giorgi <jgiorgi@chromium.org>
Reviewed-by: Simran Basi <sbasi@chromium.org>
diff --git a/cli/cli_mock.py b/cli/cli_mock.py
index e636ee9..4c9e172 100644
--- a/cli/cli_mock.py
+++ b/cli/cli_mock.py
@@ -3,10 +3,12 @@
 
 """Test for cli."""
 
-import unittest, os, sys, StringIO
+import os
+import sys
+import unittest
 
 import common
-from autotest_lib.cli import atest, topic_common, rpc
+from autotest_lib.cli import atest, rpc
 from autotest_lib.frontend.afe import rpc_client_lib
 from autotest_lib.frontend.afe.json_rpc import proxy
 from autotest_lib.client.common_lib.test_utils import mock
@@ -15,39 +17,72 @@
 CLI_USING_PDB = False
 CLI_UT_DEBUG = False
 
+
+class ExitException(Exception):
+    """Junk that should be removed."""
+    pass
+
 def create_file(content):
+    """Create a temporary file for testing.
+
+    @param content: string contents for file.
+
+    @return: Instance of autotemp.tempfile with specified contents.
+    """
     file_temp = autotemp.tempfile(unique_id='cli_mock', text=True)
     os.write(file_temp.fd, content)
     return file_temp
 
 
-class ExitException(Exception):
-    pass
-
-
 class cli_unittest(unittest.TestCase):
+    """General mocks and setup / teardown for testing the atest cli.
+    """
     def setUp(self):
+        """Setup mocks for rpc calls and system exit.
+        """
         super(cli_unittest, self).setUp()
         self.god = mock.mock_god(debug=CLI_UT_DEBUG, ut=self)
         self.god.stub_class_method(rpc.afe_comm, 'run')
         self.god.stub_function(sys, 'exit')
 
         def stub_authorization_headers(*args, **kwargs):
+            """No auth headers required for testing."""
             return {}
         self.god.stub_with(rpc_client_lib, 'authorization_headers',
                            stub_authorization_headers)
 
 
     def tearDown(self):
-        super(cli_unittest, self).tearDown()
+        """Remove mocks.
+        """
+        # Unstub first because super may need exit
         self.god.unstub_all()
+        super(cli_unittest, self).tearDown()
 
 
     def assertEqualNoOrder(self, x, y, message=None):
+        """Assert x and y contain the same elements.
+
+        @param x: list like object for comparison.
+        @param y: second list like object for comparison
+        @param message: Message for AssertionError if x and y contain different
+                        elements.
+
+        @raises: AssertionError
+        """
         self.assertEqual(set(x), set(y), message)
 
 
     def assertWords(self, string, to_find=[], not_in=[]):
+        """Assert the string contains all of the set of words to_find and none
+        of the set not_in.
+
+        @param string: String to search.
+        @param to_find: List of strings that must be in string.
+        @param not_in: List of strings that must NOT be in string.
+
+        @raises: AssertionError
+        """
         for word in to_find:
             self.assert_(string.find(word) >= 0,
                          "Could not find '%s' in: %s" % (word, string))
@@ -73,6 +108,20 @@
     def assertOutput(self, obj, results,
                      out_words_ok=[], out_words_no=[],
                      err_words_ok=[], err_words_no=[]):
+        """Assert that obj's output writes the expected strings to std(out/err).
+
+        An empty list for out_words_ok or err_words_ok means that the stdout
+        or stderr (respectively) must be empty.
+
+        @param obj: Command object (such as atest_add_or_remove).
+        @param results: Results of command for obj.output to format.
+        @param out_words_ok: List of strings that must be in stdout.
+        @param out_words_no: List of strings that must NOT be in stdout.
+        @param err_words_ok: List of strings that must be in stderr.
+        @param err_words_no: List of strings that must NOT be in stderr.
+
+        @raises: AssertionError
+        """
         self.god.mock_io()
         obj.output(results)
         obj.show_all_failures()
@@ -82,26 +131,40 @@
 
 
     def mock_rpcs(self, rpcs):
-        """rpcs is a list of tuples, each representing one RPC:
-        (op, **dargs, success, expected)"""
+        """Expect and mock the results of a list of RPCs.
+
+        @param rpcs: A list of tuples, each representing one RPC:
+                     (op, args(dict), success, expected)
+        """
         for (op, dargs, success, expected) in rpcs:
             comm = rpc.afe_comm.run
             if success:
                 comm.expect_call(op, **dargs).and_return(expected)
             else:
-                comm.expect_call(op, **dargs).and_raises(proxy.JSONRPCException(expected))
-
+                (comm.expect_call(op, **dargs).
+                 and_raises(proxy.JSONRPCException(expected)))
 
 
     def run_cmd(self, argv, rpcs=[], exit_code=None,
                 out_words_ok=[], out_words_no=[],
                 err_words_ok=[], err_words_no=[]):
-        """Runs the command in argv.
-        rpcs is a list of tuples, each representing one RPC:
-             (op, **dargs, success, expected)
-        exit_code should be set if you expect the command
-        to fail
-        The words are lists of words that are expected"""
+        """Run an atest command with arguments.
+
+        An empty list for out_words_ok or err_words_ok means that the stdout
+        or stderr (respectively) must be empty.
+
+        @param argv: List of command and arguments as strings.
+        @param rpcs: List of rpcs to expect the command to perform.
+        @param exit_code: Expected exit code of the command (if not 0).
+        @param out_words_ok: List of strings to expect in stdout.
+        @param out_words_no: List of strings that must not be in stdout.
+        @param err_words_ok: List of strings to expect in stderr.
+        @param err_words_no: List of strings that must not be in stderr.
+
+        @raises: AssertionError or CheckPlaybackError.
+
+        @returns: stdout, stderr
+        """
         sys.argv = argv
 
         self.mock_rpcs(rpcs)
@@ -118,3 +181,4 @@
         self._check_output(out, out_words_ok, out_words_no,
                            err, err_words_ok, err_words_no)
         return (out, err)
+
diff --git a/cli/host_unittest.py b/cli/host_unittest.py
index 8a824b4..beda185 100755
--- a/cli/host_unittest.py
+++ b/cli/host_unittest.py
@@ -6,12 +6,13 @@
 
 # pylint: disable=missing-docstring
 
-import unittest, sys
+import sys
+import unittest
 
 import common
 from autotest_lib.cli import cli_mock, host
 from autotest_lib.client.common_lib import control_data
-
+from autotest_lib.server import hosts
 CLIENT = control_data.CONTROL_TYPE_NAMES.CLIENT
 SERVER = control_data.CONTROL_TYPE_NAMES.SERVER
 
@@ -1273,16 +1274,14 @@
 
 class host_mod_unittest(cli_mock.cli_unittest):
     def test_execute_lock_one_host(self):
-        self.run_cmd(argv=['atest', 'host', 'mod',
-                           '--lock', 'host0', '--ignore_site_file'],
+        self.run_cmd(argv=['atest', 'host', 'mod', '--lock', 'host0'],
                      rpcs=[('modify_host', {'id': 'host0', 'locked': True},
                             True, None)],
                      out_words_ok=['Locked', 'host0'])
 
 
     def test_execute_unlock_two_hosts(self):
-        self.run_cmd(argv=['atest', 'host', 'mod',
-                           '-u', 'host0,host1', '--ignore_site_file'],
+        self.run_cmd(argv=['atest', 'host', 'mod', '-u', 'host0,host1'],
                      rpcs=[('modify_host', {'id': 'host1', 'locked': False,
                                             'lock_reason': ''},
                             True, None),
@@ -1293,9 +1292,8 @@
 
 
     def test_execute_force_lock_one_host(self):
-        self.run_cmd(argv=['atest', 'host', 'mod',
-                           '--lock', '--force_modify_locking', 'host0',
-                           '--ignore_site_file'],
+        self.run_cmd(argv=['atest', 'host', 'mod', '--lock',
+                           '--force_modify_locking', 'host0'],
                      rpcs=[('modify_host',
                             {'id': 'host0', 'locked': True,
                              'force_modify_locking': True},
@@ -1304,9 +1302,8 @@
 
 
     def test_execute_force_unlock_one_host(self):
-        self.run_cmd(argv=['atest', 'host', 'mod',
-                           '--unlock', '--force_modify_locking', 'host0',
-                           '--ignore_site_file'],
+        self.run_cmd(argv=['atest', 'host', 'mod', '--unlock',
+                           '--force_modify_locking', 'host0'],
                      rpcs=[('modify_host',
                             {'id': 'host0', 'locked': False,
                              'force_modify_locking': True,
@@ -1316,8 +1313,8 @@
 
 
     def test_execute_lock_unknown_hosts(self):
-        self.run_cmd(argv=['atest', 'host', 'mod',
-                           '-l', 'host0,host1', 'host2', '--ignore_site_file'],
+        self.run_cmd(argv=['atest', 'host', 'mod', '-l', 'host0,host1',
+                           'host2'],
                      rpcs=[('modify_host', {'id': 'host2', 'locked': True},
                             True, None),
                            ('modify_host', {'id': 'host1', 'locked': True},
@@ -1331,174 +1328,328 @@
 
     def test_execute_protection_hosts(self):
         mfile = cli_mock.create_file('host0\nhost1,host2\nhost3 host4')
-        self.run_cmd(argv=['atest', 'host', 'mod', '--protection',
-                           'Do not repair',
-                           'host5' ,'--mlist', mfile.name, 'host1', 'host6',
-                           '--ignore_site_file'],
-                     rpcs=[('modify_host', {'id': 'host6',
-                                            'protection': 'Do not repair'},
-                            True, None),
-                           ('modify_host', {'id': 'host5',
-                                            'protection': 'Do not repair'},
-                            True, None),
-                           ('modify_host', {'id': 'host4',
-                                            'protection': 'Do not repair'},
-                            True, None),
-                           ('modify_host', {'id': 'host3',
-                                            'protection': 'Do not repair'},
-                            True, None),
-                           ('modify_host', {'id': 'host2',
-                                            'protection': 'Do not repair'},
-                            True, None),
-                           ('modify_host', {'id': 'host1',
-                                            'protection': 'Do not repair'},
-                            True, None),
-                           ('modify_host', {'id': 'host0',
-                                            'protection': 'Do not repair'},
-                            True, None)],
-                     out_words_ok=['Do not repair', 'host0', 'host1', 'host2',
-                                   'host3', 'host4', 'host5', 'host6'])
-        mfile.clean()
+        try:
+            self.run_cmd(argv=['atest', 'host', 'mod', '--protection',
+                               'Do not repair', 'host5' ,'--mlist', mfile.name,
+                               'host1', 'host6'],
+                         rpcs=[('modify_host', {'id': 'host6',
+                                                'protection': 'Do not repair'},
+                                True, None),
+                               ('modify_host', {'id': 'host5',
+                                                'protection': 'Do not repair'},
+                                True, None),
+                               ('modify_host', {'id': 'host4',
+                                                'protection': 'Do not repair'},
+                                True, None),
+                               ('modify_host', {'id': 'host3',
+                                                'protection': 'Do not repair'},
+                                True, None),
+                               ('modify_host', {'id': 'host2',
+                                                'protection': 'Do not repair'},
+                                True, None),
+                               ('modify_host', {'id': 'host1',
+                                                'protection': 'Do not repair'},
+                                True, None),
+                               ('modify_host', {'id': 'host0',
+                                                'protection': 'Do not repair'},
+                                True, None)],
+                         out_words_ok=['Do not repair', 'host0', 'host1',
+                                       'host2', 'host3', 'host4', 'host5',
+                                       'host6'])
+        finally:
+            mfile.clean()
 
+    def test_execute_attribute_host(self):
+        self.run_cmd(argv=['atest', 'host', 'mod', 'host0', '--attribute',
+                           'foo=bar'],
+                     rpcs=[('modify_host', {'id': 'host0'}, True, None),
+                           ('set_host_attribute', {'hostname': 'host0',
+                                                   'attribute': 'foo',
+                                                   'value': 'bar'},
+                            True, None)],
+                     out_words_ok=[])
 
 
 class host_create_unittest(cli_mock.cli_unittest):
-    def test_execute_create_muliple_hosts_all_options(self):
-        self.run_cmd(argv=['atest', 'host', 'create', '--lock',
-                           '-b', 'label0', '--acls', 'acl0', 'host0', 'host1',
-                           '--ignore_site_file'],
-                     rpcs=[('get_labels', {'name': 'label0'},
-                            True,
-                            [{u'id': 4,
-                              u'platform': 0,
-                              u'name': u'label0',
-                              u'invalid': False,
-                              u'kernel_config': u''}]),
-                           ('get_acl_groups', {'name': 'acl0'},
-                            True, []),
-                           ('add_acl_group', {'name': 'acl0'},
-                            True, 5),
-                           ('add_host', {'hostname': 'host1',
-                                         'status': 'Ready',
-                                         'locked': True},
-                            True, 42),
-                           ('host_add_labels', {'id': 'host1',
-                                                'labels': ['label0']},
-                            True, None),
-                           ('add_host', {'hostname': 'host0',
-                                         'status': 'Ready',
-                                         'locked': True},
-                            True, 42),
-                           ('host_add_labels', {'id': 'host0',
-                                                'labels': ['label0']},
-                            True, None),
-                           ('acl_group_add_hosts',
-                            {'id': 'acl0', 'hosts': ['host1', 'host0']},
-                            True, None)],
-                     out_words_ok=['host0', 'host1'])
+    _out = ['Added', 'host', 'localhost']
+    _command = ['atest', 'host', 'create', 'localhost']
+
+    def _mock_host(self, platform=None, labels=[]):
+        mock_host = self.god.create_mock_class(hosts.Host, 'Host')
+        hosts.create_host = self.god.create_mock_function('create_host')
+        hosts.create_host.expect_any_call().and_return(mock_host)
+        mock_host.get_platform.expect_call().and_return(platform)
+        mock_host.get_labels.expect_call().and_return(labels)
+        return mock_host
 
 
-    def test_execute_create_muliple_hosts_unlocked(self):
-        self.run_cmd(argv=['atest', 'host', 'create',
-                           '-b', 'label0', '--acls', 'acl0', 'host0', 'host1',
-                           '--ignore_site_file'],
-                     rpcs=[('get_labels', {'name': 'label0'},
-                            True,
-                            [{u'id': 4,
-                              u'platform': 0,
-                              u'name': u'label0',
-                              u'invalid': False,
-                              u'kernel_config': u''}]),
-                           ('get_acl_groups', {'name': 'acl0'},
-                            True, []),
-                           ('add_acl_group', {'name': 'acl0'},
-                            True, 5),
-                           ('add_host', {'hostname': 'host1',
-                                         'status': 'Ready',
-                                         'locked': True,
-                                         'lock_reason': 'Forced lock on device creation'},
-                            True, 42),
-                           ('host_add_labels', {'id': 'host1',
-                                                'labels': ['label0']},
-                            True, None),
-                           ('add_host', {'hostname': 'host0',
-                                         'status': 'Ready',
-                                         'locked': True,
-                                         'lock_reason': 'Forced lock on device creation'},
-                            True, 42),
-                           ('host_add_labels', {'id': 'host0',
-                                                'labels': ['label0']},
-                            True, None),
-                           ('acl_group_add_hosts',
-                            {'id': 'acl0', 'hosts': ['host1', 'host0']},
-                            True, None),
-                           ('modify_host', {'id': 'host1', 'locked': False,
-                                            'lock_reason': ''},
-                            True, None),
-                           ('modify_host', {'id': 'host0', 'locked': False,
-                                            'lock_reason': ''},
-                            True, None)],
-                     out_words_ok=['host0', 'host1'])
+    def _mock_testbed(self, platform=None, labels=[]):
+        mock_tb = self.god.create_mock_class(hosts.TestBed, 'TestBed')
+        hosts.create_testbed = self.god.create_mock_function('create_testbed')
+        hosts.create_testbed.expect_any_call().and_return(mock_tb)
+        mock_tb.get_platform.expect_call().and_return(platform)
+        mock_tb.get_labels.expect_call().and_return(labels)
+        return mock_tb
 
 
-    def test_execute_create_muliple_hosts_label_escaped_quotes(self):
-        self.run_cmd(argv=['atest', 'host', 'create',
-                           '-b', 'label0,label\\,1,label\\,2',
-                           '--acls', 'acl0', 'host0', 'host1',
-                           '--ignore_site_file'],
-                     rpcs=[('get_labels', {'name': 'label0'},
-                            True,
-                            [{u'id': 4,
-                              u'platform': 0,
-                              u'name': u'label0',
-                              u'invalid': False,
-                              u'kernel_config': u''}]),
-                           ('get_labels', {'name': 'label,1'},
-                            True,
-                            [{u'id': 4,
-                              u'platform': 0,
-                              u'name': u'label,1',
-                              u'invalid': False,
-                              u'kernel_config': u''}]),
-                           ('get_labels', {'name': 'label,2'},
-                            True,
-                            [{u'id': 4,
-                              u'platform': 0,
-                              u'name': u'label,2',
-                              u'invalid': False,
-                              u'kernel_config': u''}]),
-                           ('get_acl_groups', {'name': 'acl0'},
-                            True, []),
-                           ('add_acl_group', {'name': 'acl0'},
-                            True, 5),
-                           ('add_host', {'hostname': 'host1',
-                                         'status': 'Ready',
-                                         'locked': True,
-                                         'lock_reason': 'Forced lock on device creation'},
-                            True, 42),
-                           ('host_add_labels', {'id': 'host1',
-                                                'labels': ['label0', 'label,1',
-                                                           'label,2']},
-                            True, None),
-                           ('add_host', {'hostname': 'host0',
-                                         'status': 'Ready',
-                                         'locked': True,
-                                         'lock_reason': 'Forced lock on device creation'},
-                            True, 42),
-                           ('host_add_labels', {'id': 'host0',
-                                                'labels': ['label0', 'label,1',
-                                                           'label,2']},
-                            True, None),
-                           ('acl_group_add_hosts',
-                            {'id': 'acl0', 'hosts': ['host1', 'host0']},
-                            True, None),
-                           ('modify_host', {'id': 'host1', 'locked': False,
-                                            'lock_reason': ''},
-                            True, None),
-                           ('modify_host', {'id': 'host0', 'locked': False,
-                                            'lock_reason': ''},
-                            True, None)],
-                     out_words_ok=['host0', 'host1'])
+    def _gen_rpcs_for_label(self, label, platform=False):
+        rpcs = [
+            ('get_labels', {'name': label}, True, []),
+            ('add_label', {'name': label, 'platform': platform}, True, None)
+        ]
+        return rpcs
+
+
+    def _gen_expected_rpcs(self, hosts=None, locked=False,
+                           lock_reason=None, platform=None, labels=None,
+                           acls=None, protection=None, serials=None):
+        """Build a list of expected RPC calls based on values to host command.
+
+        @param hosts: list of hostname being created (default ['localhost'])
+        @param locked: end state of host (bool)
+        @param lock_reason: reason for host to be locked
+        @param platform: platform label
+        @param labels: list of host labels (excluding platform)
+        @param acls: list of host acls
+        @param protection: host protection level
+
+        @return: list of expect rpc calls (each call is (op, args, success,
+            result))
+        """
+        rpcs = []
+        hosts = hosts[:] if hosts else ['localhost']
+        hosts.reverse() # No idea why
+        lock_reason = lock_reason or 'Forced lock on device creation'
+        acls = acls or []
+        labels = labels or []
+
+        if platform:
+            rpcs += self._gen_rpcs_for_label(platform, platform=True)
+        for label in labels:
+            rpcs += self._gen_rpcs_for_label(label) * len(hosts)
+
+        for acl in acls:
+            rpcs.append(('get_acl_groups', {'name': acl}, True, []))
+            rpcs.append(('add_acl_group', {'name': acl}, True, None))
+
+        for host in hosts:
+            add_args = {
+                'hostname': host,
+                'status': 'Ready',
+                'locked': True,
+                'lock_reason': lock_reason,
+            }
+            if protection:
+                add_args['protection'] = protection
+            rpcs.append(('add_host', add_args, True, None))
+
+            if labels or platform:
+                rpcs.append((
+                    'host_add_labels',
+                    {
+                        'id': host,
+                        'labels': labels + [platform] if platform else labels,
+                    },
+                    True,
+                    None
+                ))
+
+        if serials:
+            for host in hosts:
+                rpcs.append((
+                    'set_host_attribute',
+                    {
+                        'hostname': host,
+                        'attribute': 'serials',
+                        'value': ','.join(serials),
+                    },
+                    True,
+                    None
+                ))
+
+        for acl in acls:
+            for host in hosts:
+                rpcs.append((
+                    'acl_group_add_hosts',
+                    {
+                        'hosts': [host],
+                        'id': acl,
+                    },
+                    True,
+                    None,
+                ))
+
+        if not locked:
+            for host in hosts:
+                rpcs.append((
+                    'modify_host',
+                    {
+                        'id': host,
+                        'locked': False,
+                        'lock_reason': '',
+                    },
+                    True,
+                    None,
+                ))
+        return rpcs
+
+
+    def test_create_simple(self):
+        self._mock_host()
+        rpcs = self._gen_expected_rpcs()
+        self.run_cmd(argv=self._command, rpcs=rpcs, out_words_ok=self._out)
+
+
+    def test_create_locked(self):
+        self._mock_host()
+        lock_reason = 'Because I said so.'
+        rpcs = self._gen_expected_rpcs(locked=True,
+                                                   lock_reason=lock_reason)
+        self.run_cmd(argv=self._command + ['-l', '-r', lock_reason],
+                     rpcs=rpcs, out_words_ok=self._out)
+
+
+    def test_create_discovered_platform(self):
+        self._mock_host(platform='some_platform')
+        rpcs = self._gen_expected_rpcs(platform='some_platform')
+        self.run_cmd(argv=self._command, rpcs=rpcs, out_words_ok=self._out)
+
+
+    def test_create_specified_platform(self):
+        self._mock_host()
+        rpcs = self._gen_expected_rpcs(platform='some_platform')
+        self.run_cmd(argv=self._command + ['-t', 'some_platform'], rpcs=rpcs,
+                     out_words_ok=self._out)
+
+
+    def test_create_specified_platform_overrides_discovered_platform(self):
+        self._mock_host(platform='wrong_platform')
+        rpcs = self._gen_expected_rpcs(platform='some_platform')
+        self.run_cmd(argv=self._command + ['-t', 'some_platform'], rpcs=rpcs,
+                     out_words_ok=self._out)
+
+
+    def test_create_discovered_labels(self):
+        labels = ['label0', 'label1']
+        self._mock_host(labels=labels)
+        rpcs = self._gen_expected_rpcs(labels=labels)
+        self.run_cmd(argv=self._command, rpcs=rpcs, out_words_ok=self._out)
+
+
+    def test_create_specified_labels(self):
+        labels = ['label0', 'label1']
+        self._mock_host()
+        rpcs = self._gen_expected_rpcs(labels=labels)
+        self.run_cmd(argv=self._command + ['-b', ','.join(labels)], rpcs=rpcs,
+                     out_words_ok=self._out)
+
+
+    def test_create_specified_labels_from_file(self):
+        labels = ['label0', 'label1']
+        self._mock_host()
+        rpcs = self._gen_expected_rpcs(labels=labels)
+        labelsf = cli_mock.create_file(','.join(labels))
+        try:
+            self.run_cmd(argv=self._command + ['-B', labelsf.name], rpcs=rpcs,
+                         out_words_ok=self._out)
+        finally:
+            labelsf.clean()
+
+    def test_create_specified_discovered_labels_combine(self):
+        labels = ['label0', 'label1']
+        self._mock_host(labels=labels[0:1])
+        rpcs = self._gen_expected_rpcs(labels=labels)
+        self.run_cmd(argv=self._command + ['-b', labels[1]], rpcs=rpcs,
+                     out_words_ok=self._out)
+
+
+    def test_create_acls(self):
+        acls = ['acl0', 'acl1']
+        self._mock_host()
+        rpcs = self._gen_expected_rpcs(acls=acls)
+        self.run_cmd(argv=self._command + ['-a', ','.join(acls)], rpcs=rpcs,
+                     out_words_ok=self._out)
+
+
+    def test_create_acls_from_file(self):
+        acls = ['acl0', 'acl1']
+        self._mock_host()
+        rpcs = self._gen_expected_rpcs(acls=acls)
+        aclsf = cli_mock.create_file(','.join(acls))
+        try:
+            self.run_cmd(argv=self._command + ['-A', aclsf.name], rpcs=rpcs,
+                         out_words_ok=self._out)
+        finally:
+            aclsf.clean()
+
+
+    def test_create_protection(self):
+        protection = 'Do not repair'
+        self._mock_host()
+        rpcs = self._gen_expected_rpcs(protection=protection)
+        self.run_cmd(argv=self._command + ['-p', protection], rpcs=rpcs,
+                     out_words_ok=self._out)
+
+
+    def test_create_protection_invalid(self):
+        protection = 'Invalid protection'
+        rpcs = self._gen_expected_rpcs()
+        self.run_cmd(argv=self._command + ['-p', protection], exit_code=2,
+                     err_words_ok=['invalid', 'choice'] + protection.split())
+
+
+    def test_create_one_serial(self):
+        serial = 'device0'
+        self._mock_host()
+        rpcs = self._gen_expected_rpcs(serials=[serial])
+        self.run_cmd(argv=self._command + ['-s', serial], rpcs=rpcs,
+                     out_words_ok=self._out)
+
+
+    def test_create_multiple_serials(self):
+        serials = ['device0', 'device1']
+        self._mock_testbed()
+        rpcs = self._gen_expected_rpcs(serials=serials)
+        self.run_cmd(argv=self._command + ['-s', ','.join(serials)], rpcs=rpcs,
+                     out_words_ok=self._out)
+
+
+    def test_create_multiple_simple_hosts(self):
+        mock_host = self._mock_host()
+        hosts.create_host.expect_any_call().and_return(mock_host)
+        mock_host.get_platform.expect_call()
+        mock_host.get_labels.expect_call().and_return([])
+
+        hostnames = ['localhost', '127.0.0.1']
+        rpcs = self._gen_expected_rpcs(hosts=hostnames)
+
+        self.run_cmd(argv=['atest', 'host', 'create'] + hostnames,
+                     rpcs=rpcs[0:4],
+                     out_words_ok=['Added', 'hosts'] + hostnames)
+
+
+    def test_create_complex(self):
+        lock_reason = 'Because I said so.'
+        platform = 'some_platform'
+        labels = ['label0', 'label1', 'label2']
+        acls = ['acl0', 'acl1']
+        protection = 'Do not verify'
+        labelsf = cli_mock.create_file(labels[2])
+        aclsf = cli_mock.create_file(acls[1])
+        cmd_args = ['-l', '-r', lock_reason, '-t', platform, '-b', labels[1],
+                    '-B', labelsf.name, '-a', acls[0], '-A', aclsf.name, '-p',
+                    protection]
+        self._mock_host(labels=labels[0:1])
+        rpcs = self._gen_expected_rpcs(locked=True, lock_reason=lock_reason,
+                                       acls=acls, labels=labels,
+                                       platform=platform, protection=protection)
+
+        try:
+            self.run_cmd(argv=self._command + cmd_args, rpcs=rpcs,
+                         out_words_ok=self._out)
+        finally:
+            labelsf.clean()
+            aclsf.clean()
 
 
 if __name__ == '__main__':
diff --git a/cli/site_host.py b/cli/site_host.py
index 7f4b622..ca8a798 100644
--- a/cli/site_host.py
+++ b/cli/site_host.py
@@ -8,7 +8,6 @@
 from autotest_lib.client.bin import utils
 from autotest_lib.cli import host, rpc
 from autotest_lib.server import hosts
-from autotest_lib.server.cros.dynamic_suite import frontend_wrappers
 from autotest_lib.client.common_lib import error, host_protections
 
 
@@ -20,8 +19,8 @@
 hosts.factory.ssh_options = ''
 
 
-# pylint: disable=missing-docstring
 class site_host(host.host):
+    """Required by atest's site logic."""
     pass
 
 
@@ -96,10 +95,8 @@
             self.execute_rpc('host_add_labels', id=host, labels=list(labels))
 
         if self.serials:
-            afe = frontend_wrappers.RetryingAFE(timeout_min=5, delay_sec=10)
-            afe.set_host_attribute('serials', ','.join(self.serials),
-                                   hostname=host)
-
+            self.execute_rpc('set_host_attribute', hostname=host,
+                             attribute='serials', value=','.join(self.serials))
 
     def execute(self):
         # Check to see if the platform or any other labels can be grabbed from
diff --git a/client/common_lib/hosts/base_classes.py b/client/common_lib/hosts/base_classes.py
index 52e788d..c11d864 100644
--- a/client/common_lib/hosts/base_classes.py
+++ b/client/common_lib/hosts/base_classes.py
@@ -82,10 +82,14 @@
 
 
     def close(self):
+        """Close the connection to the host.
+        """
         pass
 
 
     def setup(self):
+        """Setup the host object.
+        """
         pass
 
 
@@ -102,7 +106,8 @@
                 to complete if it has to kill the process.
         @param ignore_status: do not raise an exception, no matter
                 what the exit code of the command is.
-        @param stdout_tee/stderr_tee: where to tee the stdout/stderr
+        @param stdout_tee: where to tee the stdout
+        @param stderr_tee: where to tee the stderr
         @param stdin: stdin to pass (a string) to the executed command
         @param args: sequence of strings to pass as arguments to command by
                 quoting them in " and escaping their contents if necessary
@@ -116,42 +121,88 @@
 
 
     def run_output(self, command, *args, **dargs):
+        """Run and retrieve the value of stdout stripped of whitespace.
+
+        @param command: Command to execute.
+        @param *args: Extra arguments to run.
+        @param **dargs: Extra keyword arguments to run.
+
+        @return: String value of stdout.
+        """
         return self.run(command, *args, **dargs).stdout.rstrip()
 
 
     def reboot(self):
+        """Reboot the host.
+        """
         raise NotImplementedError('Reboot not implemented!')
 
 
     def suspend(self):
+        """Suspend the host.
+        """
         raise NotImplementedError('Suspend not implemented!')
 
 
     def sysrq_reboot(self):
+        """Execute host reboot via SysRq key.
+        """
         raise NotImplementedError('Sysrq reboot not implemented!')
 
 
     def reboot_setup(self, *args, **dargs):
+        """Prepare for reboot.
+
+        This doesn't appear to be implemented by any current hosts.
+
+        @param *args: Extra arguments to ?.
+        @param **dargs: Extra keyword arguments to ?.
+        """
         pass
 
 
     def reboot_followup(self, *args, **dargs):
+        """Post reboot work.
+
+        This doesn't appear to be implemented by any current hosts.
+
+        @param *args: Extra arguments to ?.
+        @param **dargs: Extra keyword arguments to ?.
+        """
         pass
 
 
     def get_file(self, source, dest, delete_dest=False):
+        """Retrieve a file from the host.
+
+        @param source: Remote file path (directory, file or list).
+        @param dest: Local file path (directory, file or list).
+        @param delete_dest: Delete files in remote path that are not in local
+            path.
+        """
         raise NotImplementedError('Get file not implemented!')
 
 
     def send_file(self, source, dest, delete_dest=False):
+        """Send a file to the host.
+
+        @param source: Local file path (directory, file or list).
+        @param dest: Remote file path (directory, file or list).
+        @param delete_dest: Delete files in remote path that are not in local
+            path.
+        """
         raise NotImplementedError('Send file not implemented!')
 
 
     def get_tmp_dir(self):
+        """Create a temporary directory on the host.
+        """
         raise NotImplementedError('Get temp dir not implemented!')
 
 
     def is_up(self):
+        """Confirm the host is online.
+        """
         raise NotImplementedError('Is up not implemented!')
 
 
@@ -192,10 +243,20 @@
 
 
     def wait_up(self, timeout=None):
+        """Wait for the host to come up.
+
+        @param timeout: Max seconds to wait.
+        """
         raise NotImplementedError('Wait up not implemented!')
 
 
     def wait_down(self, timeout=None, warning_timer=None, old_boot_id=None):
+        """Wait for the host to go down.
+
+        @param timeout: Max seconds to wait before returning.
+        @param warning_timer: Seconds before warning host is not down.
+        @param old_boot_id: Result of self.get_boot_id() before shutdown.
+        """
         raise NotImplementedError('Wait down not implemented!')
 
 
@@ -217,8 +278,21 @@
                          down_timeout=WAIT_DOWN_REBOOT_TIMEOUT,
                          down_warning=WAIT_DOWN_REBOOT_WARNING,
                          log_failure=True, old_boot_id=None, **dargs):
-        """ Wait for the host to come back from a reboot. This is a generic
-        implementation based entirely on wait_up and wait_down. """
+        """Wait for the host to come back from a reboot.
+
+        This is a generic implementation based entirely on wait_up and
+        wait_down.
+
+        @param timeout: Max seconds to wait for reboot to start.
+        @param down_timeout: Max seconds to wait for host to go down.
+        @param down_warning: Seconds to wait before warning host hasn't gone
+            down.
+        @param log_failure: bool(Log when host does not go down.)
+        @param old_boot_id: Result of self.get_boot_id() before restart.
+        @param **dargs: Extra arguments to reboot_followup.
+
+        @raises AutoservRebootError if host does not come back up.
+        """
         key_string = 'Reboot.%s' % dargs.get('board')
 
         total_reboot_timer = autotest_stats.Timer('%s.total' % key_string,
@@ -250,20 +324,28 @@
 
 
     def verify(self):
+        """Check if host is in good state.
+        """
         self.verify_hardware()
         self.verify_connectivity()
         self.verify_software()
 
 
     def verify_hardware(self):
+        """Check host hardware.
+        """
         pass
 
 
     def verify_connectivity(self):
+        """Check host network connectivity.
+        """
         pass
 
 
     def verify_software(self):
+        """Check host software.
+        """
         pass
 
 
@@ -316,7 +398,12 @@
 
 
     def erase_dir_contents(self, path, ignore_status=True, timeout=3600):
-        """Empty a given directory path contents."""
+        """Empty a given directory path contents.
+
+        @param path: Path to empty.
+        @param ignore_status: Ignore the exit status from run.
+        @param timeout: Max seconds to allow command to complete.
+        """
         rm_cmd = 'find "%s" -mindepth 1 -maxdepth 1 -print0 | xargs -0 rm -rf'
         self.run(rm_cmd % path, ignore_status=ignore_status, timeout=timeout)
 
@@ -341,14 +428,23 @@
 
 
     def cleanup(self):
+        """Restore host to clean state.
+        """
         pass
 
 
     def machine_install(self):
+        """Install on the host.
+        """
         raise NotImplementedError('Machine install not implemented!')
 
 
     def install(self, installableObject):
+        """Call install on a thing.
+
+        @param installableObject: Thing with install method that will accept our
+            self.
+        """
         installableObject.install(self)
 
 
@@ -417,7 +513,11 @@
 
 
     def path_exists(self, path):
-        """ Determine if path exists on the remote machine. """
+        """Determine if path exists on the remote machine.
+
+        @param path: path to check
+
+        @return: bool(path exists)"""
         result = self.run('ls "%s" > /dev/null' % utils.sh_escape(path),
                           ignore_status=True)
         return result.exit_status == 0
@@ -463,8 +563,11 @@
 
 
     def list_files_glob(self, glob):
-        """
-        Get a list of files on a remote host given a glob pattern path.
+        """Get a list of files on a remote host given a glob pattern path.
+
+        @param glob: pattern
+
+        @return: list of files
         """
         SCRIPT = ("python -c 'import cPickle, glob, sys;"
                   "cPickle.dump(glob.glob(sys.argv[1]), sys.stdout, 0)'")
@@ -585,3 +688,21 @@
         adb_serial, so this method should be overriden in ADBHost.
         """
         return ['job_repo_url']
+
+
+    def get_platform(self):
+        """Determine the correct platform label for this host.
+
+        @return: A string representing this host's platform.
+        """
+        raise NotImplementedError("Get platform not implemented!")
+
+
+    def get_labels(self):
+        """Return a list of the labels gathered from the devices connected.
+
+        @return: A list of strings that denote the labels from all the devices
+        connected.
+        """
+        raise NotImplementedError("Get labels not implemented!")
+