Refactor code and add more unit tests

Refactoring was done to make more of the methods easier to test.
Existing unit tests were adjusted/simplified and more tests were
added as a result.

Change-Id: I57c334c6a11fa8e12c88bdd0788d63a35cdfd7bc

refstack_client/refstack_client.py

@@ -55,41 +55,35 @@ class RefstackClient:
         else:
             self.logger.setLevel(logging.ERROR)
 
-        # assign local vars to match args
+        self.args = args
-        self.url = args.url
+        self.tempest_script = os.path.join(self.args.tempest_dir,
-        self.tempest_dir = args.tempest_dir
+                                           'run_tempest.sh')
-        self.tempest_script = os.path.join(self.tempest_dir, 'run_tempest.sh')
-        self.test_cases = args.test_cases
-        self.verbose = args.verbose
-        self.offline = args.offline
 
         # Check that the config file exists.
-        if not os.path.isfile(args.conf_file):
+        if not os.path.isfile(self.args.conf_file):
-            self.logger.error("File not valid: %s" % args.conf_file)
+            self.logger.error("Conf file not valid: %s" % self.args.conf_file)
             exit(1)
 
-        self.conf_file = args.conf_file
+        # Check that the Tempest directory is an existing directory.
+        if not os.path.isdir(self.args.tempest_dir):
+            self.logger.error("Tempest directory given is not a directory or "
+                              "does not exist: %s" % self.args.tempest_dir)
+            exit(1)
+
+        self.tempest_script = os.path.join(self.args.tempest_dir,
+                                           'run_tempest.sh')
+
+        self.conf_file = self.args.conf_file
         self.conf = ConfigParser.SafeConfigParser()
-        self.conf.read(self.conf_file)
+        self.conf.read(self.args.conf_file)
 
-        self.results_file = self._get_subunit_output_file()
+    def _get_next_stream_subunit_output_file(self, tempest_dir):
-        self.cpid = self._get_cpid_from_keystone()
-
-    def post_results(self, content):
-        '''Post the combined results back to the server.'''
-
-        # TODO(cdiep): Post results once the API is available as outlined here:
-        # github.com/stackforge/refstack/blob/master/specs/approved/api-v1.md
-
-        self.logger.debug('API request content: %s ' % content)
-
-    def _get_subunit_output_file(self):
         '''This method reads from the next-stream file in the .testrepository
            directory of the given Tempest path. The integer here is the name
            of the file where subunit output will be saved to.'''
         try:
             subunit_file = open(os.path.join(
-                                self.tempest_dir, '.testrepository',
+                                tempest_dir, '.testrepository',
                                 'next-stream'), 'r').read().rstrip()
         except (IOError, OSError):
             self.logger.debug('The .testrepository/next-stream file was not '
@@ -100,20 +94,20 @@ class RefstackClient:
             # there is a newly generated .testrepository directory.
             subunit_file = "0"
 
-        return os.path.join(self.tempest_dir, '.testrepository', subunit_file)
+        return os.path.join(tempest_dir, '.testrepository', subunit_file)
 
-    def _get_cpid_from_keystone(self):
+    def _get_cpid_from_keystone(self, conf_file):
         '''This will get the Keystone service ID which is used as the CPID.'''
         try:
-            args = {'auth_url': self.conf.get('identity', 'uri'),
+            args = {'auth_url': conf_file.get('identity', 'uri'),
-                    'username': self.conf.get('identity', 'admin_username'),
+                    'username': conf_file.get('identity', 'admin_username'),
-                    'password': self.conf.get('identity', 'admin_password')}
+                    'password': conf_file.get('identity', 'admin_password')}
 
             if self.conf.has_option('identity', 'admin_tenant_id'):
-                args['tenant_id'] = self.conf.get('identity',
+                args['tenant_id'] = conf_file.get('identity',
                                                   'admin_tenant_id')
             else:
-                args['tenant_name'] = self.conf.get('identity',
+                args['tenant_name'] = conf_file.get('identity',
                                                     'admin_tenant_name')
 
             client = ksclient.Client(**args)
@@ -127,38 +121,54 @@ class RefstackClient:
             self.logger.error("Invalid Config File: %s" % e)
             exit(1)
 
-    def _form_json_content(self, cpid, duration, results):
+    def _form_result_content(self, cpid, duration, results):
-        '''This method will create the JSON content for the request.'''
+        '''This method will create the content for the request. The spec at
+           github.com/stackforge/refstack/blob/master/specs/approved/api-v1.md.
+           defines the format expected by the API.'''
         content = {}
         content['cpid'] = cpid
         content['duration_seconds'] = duration
         content['results'] = results
-        return json.dumps(content)
+        return content
 
     def get_passed_tests(self, result_file):
-        '''Get just the tests IDs in a subunit file that passed Tempest.'''
+        '''Get a list of tests IDs that passed Tempest from a subunit file.'''
         subunit_processor = SubunitProcessor(result_file)
         results = subunit_processor.process_stream()
         return results
 
+    def post_results(self, url, content):
+        '''Post the combined results back to the server.'''
+
+        # TODO(cdiep): Post results once the API is available as outlined here:
+        # github.com/stackforge/refstack/blob/master/specs/approved/api-v1.md
+        json_content = json.dumps(content)
+        self.logger.debug('API request content: %s ' % json_content)
+
     def run(self):
         '''Execute tempest test against the cloud.'''
         try:
+            results_file = self._get_next_stream_subunit_output_file(
+                self.args.tempest_dir)
+            cpid = self._get_cpid_from_keystone(self.conf)
+
             self.logger.info("Starting Tempest test...")
             start_time = time.time()
 
-            # Run the tempest script, specifying the conf file and the
+            # Run the tempest script, specifying the conf file, the flag
-            # flag telling it to not use a virtual environment.
+            # telling it to not use a virtual environment (-N), and the flag
+            # telling it to run the tests serially (-t).
             cmd = (self.tempest_script, '-C', self.conf_file, '-N', '-t')
 
-            # Add the tempest test cases to test as arguments.
+            # Add the tempest test cases to test as arguments. If no test
-            if self.test_cases:
+            # cases are specified, then all Tempest API tests will be run.
-                cmd += ('--', self.test_cases)
+            if self.args.test_cases:
+                cmd += ('--', self.args.test_cases)
             else:
                 cmd += ('--', "tempest.api")
 
             # If there were two verbose flags, show tempest results.
-            if self.verbose > 1:
+            if self.args.verbose > 1:
                 stderr = None
             else:
                 # Suppress tempest results output. Note that testr prints
@@ -174,17 +184,16 @@ class RefstackClient:
             duration = int(elapsed)
 
             self.logger.info('Tempest test complete.')
-            self.logger.info('Subunit results located in: %s' %
+            self.logger.info('Subunit results located in: %s' % results_file)
-                             self.results_file)
 
-            results = self.get_passed_tests(self.results_file)
+            results = self.get_passed_tests(results_file)
             self.logger.info("Number of passed tests: %d" % len(results))
 
             # If the user did not specify the offline argument, then upload
             # the results.
-            if not self.offline:
+            if not self.args.offline:
-                content = self._form_json_content(self.cpid, duration, results)
+                content = self._form_result_content(cpid, duration, results)
-                self.post_results(content)
+                self.post_results(self.args.url, content)
 
         except subprocess.CalledProcessError as e:
             self.logger.error('%s failed to complete' % (e))

refstack_client/tests/unit/.testrepository/0

@@ -1,7 +1,23 @@
 time: 2014-08-15 10:34:49.010492Z
 tags: worker-0
-test: refstack_client.tests.unit.tests.TestSequenceFunctions.test_run
+test: tempest.passed.test
-time: 2014-08-15 10:34:49.020584Z
+time: 2014-08-15 10:34:51.020584Z
-successful: refstack_client.tests.unit.tests.TestSequenceFunctions.test_run [ multipart
+successful: tempest.passed.test [ multipart
 ]
 tags: -worker-0
+time: 2014-08-15 10:34:57.543400Z
+tags: worker-0
+test: tempest.failed.test
+time: 2014-08-15 10:34:57.548225Z
+failure: tempest.failed.test [ multipart
+Content-Type: text/x-traceback;charset="utf8",language="python"
+traceback
+2E4
+Traceback (most recent call last):
+  File "/usr/lib/python2.7/some_file.py", line 2335, in exit
+    _sys.exit(status)
+SystemExit: 2
+0
+]
+tags: -worker-0
+

refstack_client/tests/unit/.testrepository/next-stream

@@ -0,0 +1 @@
+1

refstack_client/tests/unit/test_client.py

@@ -13,11 +13,10 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 #
-import ConfigParser
+
 import logging
 import os
 import subprocess
-import tempfile
 
 import mock
 from mock import MagicMock
@@ -27,6 +26,10 @@ import refstack_client.refstack_client as rc
 
 
 class TestRefstackClient(unittest.TestCase):
+
+    test_path = os.path.dirname(os.path.realpath(__file__))
+    conf_file_name = '%s/refstack-client.test.conf' % test_path
+
     def patch(self, name, **kwargs):
         """
         :param name: Name of class to be patched
@@ -38,33 +41,29 @@ class TestRefstackClient(unittest.TestCase):
         self.addCleanup(patcher.stop)
         return thing
 
-    def mock_argv(self, conf_file_name=None, verbose=None):
+    def mock_argv(self, conf_file_name=None, tempest_dir=None, verbose=None):
         """
-        Build argv for test
+        Build argv for test.
         :param conf_file_name: Configuration file name
         :param verbose: verbosity level
         :return: argv
         """
         if conf_file_name is None:
             conf_file_name = self.conf_file_name
+        if tempest_dir is None:
+            tempest_dir = self.test_path
         argv = ['-c', conf_file_name,
-                '--tempest-dir', self.test_path,
+                '--tempest-dir', tempest_dir,
                 '--test-cases', 'tempest.api.compute',
                 '--url', '0.0.0.0']
         if verbose:
             argv.append(verbose)
         return argv
 
-    def setUp(self):
+    def mock_keystone(self):
         """
-        Test case setup
+        Mock the Keystone client methods.
         """
-        logging.disable(logging.CRITICAL)
-        self.mock_tempest_process = MagicMock(name='tempest_runner')
-        self.mock_popen = self.patch(
-            'refstack_client.refstack_client.subprocess.Popen',
-            return_value=self.mock_tempest_process
-        )
         self.mock_identity_service = MagicMock(
             name='service', **{'type': 'identity', 'id': 'test-id'})
         self.mock_ks_client = MagicMock(
@@ -75,12 +74,16 @@ class TestRefstackClient(unittest.TestCase):
             'refstack_client.refstack_client.ksclient.Client',
             return_value=self.mock_ks_client
         )
-        self.test_path = os.path.dirname(os.path.realpath(__file__))
+
-        self.conf_file_name = '%s/refstack-client.test.conf' % self.test_path
+    def setUp(self):
+        """
+        Test case setup
+        """
+        logging.disable(logging.CRITICAL)
 
     def test_verbose(self):
         """
-        Test different verbosity levels
+        Test different verbosity levels.
         """
         args = rc.parse_cli_args(self.mock_argv())
         client = rc.RefstackClient(args)
@@ -94,76 +97,176 @@ class TestRefstackClient(unittest.TestCase):
         client = rc.RefstackClient(args)
         self.assertEqual(client.logger.level, logging.DEBUG)
 
-    def test_no_conf_file(self):
+    def test_get_next_stream_subunit_output_file(self):
         """
-        Test not existing configuration file
+        Test getting the subunit file from an existing .testrepository
+        directory that has a next-stream file.
         """
-        args = rc.parse_cli_args(self.mock_argv(conf_file_name='ptn-khl'))
+        args = rc.parse_cli_args(self.mock_argv())
-        self.assertRaises(SystemExit, rc.RefstackClient, args)
-
-    def test_run_with_default_config(self):
-        """
-        Test run with default configuration.
-        admin_tenant_id is set in configuration.
-        """
-        args = rc.parse_cli_args(self.mock_argv(verbose='-vv'))
         client = rc.RefstackClient(args)
-        client.run()
+        output_file = client._get_next_stream_subunit_output_file(
-        self.mock_popen.assert_called_with(
+            self.test_path)
-            ('%s/run_tempest.sh' % self.test_path, '-C', self.conf_file_name,
+
-             '-N', '-t', '--', 'tempest.api.compute'),
+        # The next-stream file contains a "1".
-            stderr=None
+        expected_file = expected_file = self.test_path + "/.testrepository/1"
-        )
+        self.assertEqual(expected_file, output_file)
+
+    def test_get_next_stream_subunit_output_file_nonexistent(self):
+        """
+        Test getting the subunit output file from a nonexistent
+        .testrepository directory.
+        """
+        args = rc.parse_cli_args(self.mock_argv())
+        client = rc.RefstackClient(args)
+        output_file = client._get_next_stream_subunit_output_file(
+            "/tempest/path")
+        expected_file = "/tempest/path/.testrepository/0"
+        self.assertEqual(expected_file, output_file)
+
+    def test_get_cpid_from_keystone_with_tenant_id(self):
+        """
+        Test getting the CPID from Keystone using an admin tenant ID.
+        """
+        args = rc.parse_cli_args(self.mock_argv())
+        client = rc.RefstackClient(args)
+        self.mock_keystone()
+        cpid = client._get_cpid_from_keystone(client.conf)
         self.ks_client_builder.assert_called_with(
             username='admin', tenant_id='admin_tenant_id',
             password='test', auth_url='0.0.0.0:35357'
         )
-        self.assertEqual('test-id', client.cpid)
+        self.assertEqual('test-id', cpid)
 
-    def test_run_with_admin_tenant_name(self):
+    def test_get_cpid_from_keystone_with_tenant_name(self):
         """
-        Test run with admin default configuration.
+        Test getting the CPID from Keystone using an admin tenant name.
-        admin_tenant_name is set in configuration.
         """
-        base_conf = ConfigParser.SafeConfigParser()
+        args = rc.parse_cli_args(self.mock_argv())
-        base_conf.read(self.conf_file_name)
-        base_conf.remove_option('identity', 'admin_tenant_id')
-        base_conf.set('identity', 'admin_tenant_name', 'admin_tenant_name')
-        test_conf = tempfile.NamedTemporaryFile()
-        base_conf.write(test_conf)
-        test_conf.flush()
-        args = rc.parse_cli_args(self.mock_argv(conf_file_name=test_conf.name,
-                                                verbose='-vv'))
         client = rc.RefstackClient(args)
-        client.run()
+        client.conf.remove_option('identity', 'admin_tenant_id')
+        client.conf.set('identity', 'admin_tenant_name', 'admin_tenant_name')
+        self.mock_keystone()
+        cpid = client._get_cpid_from_keystone(client.conf)
         self.ks_client_builder.assert_called_with(
             username='admin', tenant_name='admin_tenant_name',
             password='test', auth_url='0.0.0.0:35357'
         )
-        self.assertEqual('test-id', client.cpid)
 
-    def test_check_admin_tenant(self):
+        self.assertEqual('test-id', cpid)
+
+    def test_get_cpid_from_keystone_no_admin_tenant(self):
         """
-        Test exit under absence information about admin tenant info
+        Test exit under absence of information about admin tenant info.
         """
-        base_conf = ConfigParser.SafeConfigParser()
+        args = rc.parse_cli_args(self.mock_argv(verbose='-vv'))
-        base_conf.read(self.conf_file_name)
+        client = rc.RefstackClient(args)
-        base_conf.remove_option('identity', 'admin_tenant_id')
+        client.conf.remove_option('identity', 'admin_tenant_id')
-        test_conf = tempfile.NamedTemporaryFile()
+        self.assertRaises(SystemExit, client._get_cpid_from_keystone,
-        base_conf.write(test_conf)
+                          client.conf)
-        test_conf.flush()
+
-        args = rc.parse_cli_args(self.mock_argv(conf_file_name=test_conf.name,
+    def test_form_result_content(self):
-                                                verbose='-vv'))
+        """
+        Test that the request content is formed into the expected format.
+        """
+        args = rc.parse_cli_args(self.mock_argv())
+        client = rc.RefstackClient(args)
+        content = client._form_result_content(1, 1, ['tempest.sample.test'])
+        expected = {'cpid': 1,
+                    'duration_seconds': 1,
+                    'results': ['tempest.sample.test']}
+        self.assertEqual(expected, content)
+
+    def test_get_passed_tests(self):
+        """
+        Test that only passing tests are retrieved from a subunit file.
+        """
+        args = rc.parse_cli_args(self.mock_argv())
+        client = rc.RefstackClient(args)
+        subunit_file = self.test_path + "/.testrepository/0"
+        results = client.get_passed_tests(subunit_file)
+        expected = ['tempest.passed.test']
+        self.assertEqual(expected, results)
+
+    def test_run_tempest(self):
+        """
+        Test that the test command will run the tempest script using the
+        default configuration.
+        """
+        args = rc.parse_cli_args(self.mock_argv(verbose='-vv'))
+        client = rc.RefstackClient(args)
+
+        mock_popen = self.patch(
+            'refstack_client.refstack_client.subprocess.Popen',
+            return_value=MagicMock())
+        self.mock_keystone()
+        client.get_passed_tests = MagicMock(return_value=['test'])
+        client.post_results = MagicMock()
+        client._save_json_results = MagicMock()
+        client.run()
+        mock_popen.assert_called_with(
+            ('%s/run_tempest.sh' % self.test_path, '-C', self.conf_file_name,
+             '-N', '-t', '--', 'tempest.api.compute'),
+            stderr=None
+        )
+
+        expected_content = {'duration_seconds': mock.ANY,
+                            'cpid': 'test-id',
+                            'results': ['test']}
+        client.post_results.assert_called_with('0.0.0.0', expected_content)
+
+    def test_run_tempest_offline(self):
+        """
+        Test that the test command will run the tempest script in offline mode.
+        """
+        argv = self.mock_argv(verbose='-vv')
+        argv.append('--offline')
+        args = rc.parse_cli_args(argv)
+        client = rc.RefstackClient(args)
+
+        mock_popen = self.patch(
+            'refstack_client.refstack_client.subprocess.Popen',
+            return_value=MagicMock())
+        self.mock_keystone()
+        client.get_passed_tests = MagicMock(return_value=['test'])
+        client.post_results = MagicMock()
+        client._save_json_results = MagicMock()
+        client.run()
+        mock_popen.assert_called_with(
+            ('%s/run_tempest.sh' % self.test_path, '-C', self.conf_file_name,
+             '-N', '-t', '--', 'tempest.api.compute'),
+            stderr=None
+        )
+
+        # The method post_results should not be called if --offline was
+        # specified.
+        self.assertFalse(client.post_results.called)
+
+    def test_run_tempest_no_conf_file(self):
+        """
+        Test when a nonexistent configuration file is passed in.
+        """
+        args = rc.parse_cli_args(self.mock_argv(conf_file_name='ptn-khl'))
+        self.assertRaises(SystemExit, rc.RefstackClient, args)
+
+    def test_run_tempest_nonexisting_directory(self):
+        """
+        Test when a nonexistent Tempest directory is passed in.
+        """
+        args = rc.parse_cli_args(self.mock_argv(tempest_dir='/does/not/exist'))
         self.assertRaises(SystemExit, rc.RefstackClient, args)
 
     def test_failed_run(self):
         """
-        Test failed tempest run
+        Test failed tempest run.
         """
-        self.mock_tempest_process.communicate = MagicMock(
+        mock_tempest_process = MagicMock(name='tempest_runner')
+        self.patch('refstack_client.refstack_client.subprocess.Popen',
+                   return_value=mock_tempest_process)
+        mock_tempest_process.communicate = MagicMock(
             side_effect=subprocess.CalledProcessError(returncode=1,
                                                       cmd='./run_tempest.sh')
         )
+        self.mock_keystone()
         args = rc.parse_cli_args(self.mock_argv(verbose='-vv'))
         client = rc.RefstackClient(args)
         self.assertEqual(client.logger.level, logging.DEBUG)