From 768b4676a1d1717efb2a9f52c4b57e56e396abd8 Mon Sep 17 00:00:00 2001 From: Paul Van Eck Date: Thu, 2 Oct 2014 00:28:58 -0700 Subject: [PATCH] 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 | 103 ++++---- refstack_client/tests/unit/.testrepository/0 | 24 +- .../tests/unit/.testrepository/next-stream | 1 + refstack_client/tests/unit/test_client.py | 223 +++++++++++++----- 4 files changed, 240 insertions(+), 111 deletions(-) create mode 100644 refstack_client/tests/unit/.testrepository/next-stream diff --git a/refstack_client/refstack_client.py b/refstack_client/refstack_client.py index 7d4d57f..cf4d88d 100755 --- a/refstack_client/refstack_client.py +++ b/refstack_client/refstack_client.py @@ -55,41 +55,35 @@ class RefstackClient: else: self.logger.setLevel(logging.ERROR) - # assign local vars to match args - self.url = args.url - self.tempest_dir = args.tempest_dir - 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 + self.args = args + self.tempest_script = os.path.join(self.args.tempest_dir, + 'run_tempest.sh') # Check that the config file exists. - if not os.path.isfile(args.conf_file): - self.logger.error("File not valid: %s" % args.conf_file) + if not os.path.isfile(self.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() - 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): + def _get_next_stream_subunit_output_file(self, tempest_dir): '''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'), - 'username': self.conf.get('identity', 'admin_username'), - 'password': self.conf.get('identity', 'admin_password')} + args = {'auth_url': conf_file.get('identity', 'uri'), + 'username': conf_file.get('identity', 'admin_username'), + '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): - '''This method will create the JSON content for the request.''' + def _form_result_content(self, cpid, duration, results): + '''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 - # flag telling it to not use a virtual environment. + # Run the tempest script, specifying the conf file, the flag + # 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. - if self.test_cases: - cmd += ('--', self.test_cases) + # Add the tempest test cases to test as arguments. If no test + # cases are specified, then all Tempest API tests will be run. + 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.results_file) + self.logger.info('Subunit results located in: %s' % 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: - content = self._form_json_content(self.cpid, duration, results) - self.post_results(content) + if not self.args.offline: + content = self._form_result_content(cpid, duration, results) + self.post_results(self.args.url, content) except subprocess.CalledProcessError as e: self.logger.error('%s failed to complete' % (e)) diff --git a/refstack_client/tests/unit/.testrepository/0 b/refstack_client/tests/unit/.testrepository/0 index 7e1c11e..a47e06a 100644 --- a/refstack_client/tests/unit/.testrepository/0 +++ b/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 -time: 2014-08-15 10:34:49.020584Z -successful: refstack_client.tests.unit.tests.TestSequenceFunctions.test_run [ multipart +test: tempest.passed.test +time: 2014-08-15 10:34:51.020584Z +successful: tempest.passed.test [ multipart ] -tags: -worker-0 \ No newline at end of file +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 + diff --git a/refstack_client/tests/unit/.testrepository/next-stream b/refstack_client/tests/unit/.testrepository/next-stream new file mode 100644 index 0000000..d00491f --- /dev/null +++ b/refstack_client/tests/unit/.testrepository/next-stream @@ -0,0 +1 @@ +1 diff --git a/refstack_client/tests/unit/test_client.py b/refstack_client/tests/unit/test_client.py index 5f33478..411ec70 100755 --- a/refstack_client/tests/unit/test_client.py +++ b/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')) - 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')) + args = rc.parse_cli_args(self.mock_argv()) client = rc.RefstackClient(args) - client.run() - self.mock_popen.assert_called_with( - ('%s/run_tempest.sh' % self.test_path, '-C', self.conf_file_name, - '-N', '-t', '--', 'tempest.api.compute'), - stderr=None - ) + output_file = client._get_next_stream_subunit_output_file( + self.test_path) + + # The next-stream file contains a "1". + 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. - admin_tenant_name is set in configuration. + Test getting the CPID from Keystone using an admin tenant name. """ - base_conf = ConfigParser.SafeConfigParser() - 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')) + args = rc.parse_cli_args(self.mock_argv()) 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() - base_conf.read(self.conf_file_name) - base_conf.remove_option('identity', 'admin_tenant_id') - 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')) + args = rc.parse_cli_args(self.mock_argv(verbose='-vv')) + client = rc.RefstackClient(args) + client.conf.remove_option('identity', 'admin_tenant_id') + self.assertRaises(SystemExit, client._get_cpid_from_keystone, + client.conf) + + def test_form_result_content(self): + """ + 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)