From 8ce9d735d25d209a747b8f29c614236b74f95591 Mon Sep 17 00:00:00 2001 From: Zhao Chao Date: Sat, 24 Feb 2018 14:56:32 +0800 Subject: [PATCH] Fix guestagent.test_operating_system for Python3 * Tests for _write_file_as_root failed because tempfile.NamedTemporaryFile use binary mode by default, changing to text mode fixes. * Migrate to olso_serialization.base64 for Base64Codec. Also change the return value from bytearray to bytes for Base64Codec.deserialize. Base64Codec supports reverse encoding(i.e. binary data will be written to the dest file), in this situation, the dest file should be opened with binary mode(and when reading from the file, binary mode should also be used). * stream_codecs.StringConverter converts iterable objects with map function. The behavior of map function is different under Python 2.x and 3.x. However csv.writerows(before Python 3.5) and unpack_singleton in trove.common.utils both need a list object. Converting the return value of map function to a list explicitly before passing to these functions fixes. guestagent.test_operating_system is the last blacklist regex pattern for py3 unittests. With the above problems fixed, blacklist-py3.txt is not needed any more, and tox.ini is also updateted. Migrating from ostestr to stestr is also done while updating tox.ini. Here is the ML post for more information about os-testr and stestr: http://lists.openstack.org/pipermail/openstack-dev/2017-September/122135.html Partially implements: blueprint trove-python3 Change-Id: I31f1f97901d6ebff8a91c1b70a343e724ab806eb Signed-off-by: Zhao Chao --- .stestr.conf | 4 +- .testr.conf | 4 - blacklist-py3.txt | 3 - tox.ini | 13 +- trove/common/crypto_utils.py | 5 +- trove/common/stream_codecs.py | 48 +++-- trove/guestagent/common/operating_system.py | 68 +++---- .../guestagent/test_operating_system.py | 171 +++++++++++++++++- 8 files changed, 244 insertions(+), 72 deletions(-) delete mode 100644 .testr.conf delete mode 100644 blacklist-py3.txt diff --git a/.stestr.conf b/.stestr.conf index f80c78cc68..6d13b42067 100644 --- a/.stestr.conf +++ b/.stestr.conf @@ -1,3 +1,3 @@ [DEFAULT] -test_path=${OS_TEST_PATH:-./trove/tests} -top_dir=./ \ No newline at end of file +test_path=${OS_TEST_PATH:-./trove/tests/unittests} +top_dir=./ diff --git a/.testr.conf b/.testr.conf deleted file mode 100644 index 9a61386854..0000000000 --- a/.testr.conf +++ /dev/null @@ -1,4 +0,0 @@ -[DEFAULT] -test_command=${PYTHON:-python} -m subunit.run discover ./trove/tests/unittests $LISTOPT $IDOPTION -test_id_option=--load-list $IDFILE -test_list_option=--list diff --git a/blacklist-py3.txt b/blacklist-py3.txt deleted file mode 100644 index c49759ede9..0000000000 --- a/blacklist-py3.txt +++ /dev/null @@ -1,3 +0,0 @@ -# Use a blacklist of tests known to fail on Python 3, until -# all unit tests will pass on Python 3. -guestagent.test_operating_system diff --git a/tox.ini b/tox.ini index 9da31853e1..817b6dd0df 100644 --- a/tox.ini +++ b/tox.ini @@ -32,20 +32,21 @@ commands = flake8 doc8 {posargs} +[stestr-base] +commands = stestr run --serial '{posargs}' + stestr slowest + [testenv:py27] commands = {[testenv]commands} - ostestr {posargs} --slowest --serial - -[py3base] -commands = ostestr --slowest --blacklist-file=blacklist-py3.txt --serial --regex '.*' + {[stestr-base]commands} [testenv:py34] commands = {[testenv]commands} - {[py3base]commands} + {[stestr-base]commands} [testenv:py35] commands = {[testenv]commands} - {[py3base]commands} + {[stestr-base]commands} [testenv:apiexamples] commands = {envpython} generate_examples.py diff --git a/trove/common/crypto_utils.py b/trove/common/crypto_utils.py index 9e3d5613bc..2c61b3f850 100644 --- a/trove/common/crypto_utils.py +++ b/trove/common/crypto_utils.py @@ -31,8 +31,9 @@ IV_BIT_COUNT = 16 def encode_data(data): - if isinstance(data, six.text_type): - data = data.encode('utf-8') + # NOTE(zhaochao) No need to encoding string object any more, + # as Base64Codec is now using oslo_serialization.base64 which + # could take care of this. return stream_codecs.Base64Codec().serialize(data) diff --git a/trove/common/stream_codecs.py b/trove/common/stream_codecs.py index e6bb41b593..752dbb9ae6 100644 --- a/trove/common/stream_codecs.py +++ b/trove/common/stream_codecs.py @@ -15,7 +15,6 @@ import abc import ast -import base64 import csv import json import re @@ -26,6 +25,8 @@ from six.moves import configparser import xmltodict import yaml +from oslo_serialization import base64 + from trove.common import utils as trove_utils @@ -324,9 +325,14 @@ class PropertiesCodec(StreamCodec): key = row[0].strip() # Ignore comment lines. if not key.strip().startswith(self._comment_markers): - items = self._string_converter.to_objects( + # NOTE(zhaochao): a list object is expected for + # trove_utils.unpack_singleton, however in python3 + # map objects won't be treated as lists, so we + # convert the result of StringConverter.to_objects + # to a list explicitly. + items = list(self._string_converter.to_objects( [v if v else None for v in - map(self._strip_comments, row[1:])]) + map(self._strip_comments, row[1:])])) current = data_dict.get(key) if current is not None: current.append(trove_utils.unpack_singleton(items) @@ -360,9 +366,14 @@ class PropertiesCodec(StreamCodec): header, self._string_converter.to_strings(items))) else: # This is a single-row property with only one argument. + # Note(zhaochao): csv.writerows expects a list object before + # python 3.5, but map objects won't be treated as lists in + # python 3, so we explicitly convert the result of + # StringConverter.to_strings to a list here to support py34 + # unittests. rows.append( - self._string_converter.to_strings( - self._to_list(header, items))) + list(self._string_converter.to_strings( + self._to_list(header, items)))) return rows @@ -434,7 +445,14 @@ class KeyValueCodec(StreamCodec): return self._line_terminator.join(lines) def deserialize(self, stream): - lines = stream.split(self._line_terminator) + # Note(zhaochao): In Python 3, when files are opened in text mode, + # newlines will be translated to '\n' by default, so we just split + # the stream by '\n'. + if sys.version_info[0] >= 3: + lines = stream.split('\n') + else: + lines = stream.split(self._line_terminator) + result = {} for line in lines: line = line.lstrip().rstrip() @@ -500,22 +518,14 @@ class Base64Codec(StreamCodec): binary data before writing to a file as well. """ - def serialize(self, data): + # NOTE(zhaochao): migrate to oslo_serialization.base64 to serialize(return + # a text object) and deserialize(return a bytes object) data. - try: - # py27str - if we've got text data, this should encode it - # py27aa/py34aa - if we've got a bytearray, this should work too - encoded = str(base64.b64encode(data).decode('utf-8')) - except TypeError: - # py34str - convert to bytes first, then we can encode - data_bytes = bytes([ord(item) for item in data]) - encoded = base64.b64encode(data_bytes).decode('utf-8') - return encoded + def serialize(self, data): + return base64.encode_as_text(data) def deserialize(self, stream): - - # py27 & py34 seem to understand bytearray the same - return bytearray([item for item in base64.b64decode(stream)]) + return base64.decode_as_bytes(stream) class XmlCodec(StreamCodec): diff --git a/trove/guestagent/common/operating_system.py b/trove/guestagent/common/operating_system.py index 282d8857b9..a210fdf537 100644 --- a/trove/guestagent/common/operating_system.py +++ b/trove/guestagent/common/operating_system.py @@ -56,13 +56,18 @@ def read_file(path, codec=IdentityCodec(), as_root=False, decode=True): :raises: :class:`UnprocessableEntity` if codec not given. """ if path and exists(path, is_directory=False, as_root=as_root): - if as_root: - return _read_file_as_root(path, codec, decode=decode) + if decode: + open_flag = 'r' + convert_func = codec.deserialize + else: + open_flag = 'rb' + convert_func = codec.serialize - with open(path, 'r') as fp: - if decode: - return codec.deserialize(fp.read()) - return codec.serialize(fp.read()) + if as_root: + return _read_file_as_root(path, open_flag, convert_func) + + with open(path, open_flag) as fp: + return convert_func(fp.read()) raise exception.UnprocessableEntity(_("File does not exist: %s") % path) @@ -97,24 +102,22 @@ def exists(path, is_directory=False, as_root=False): return found -def _read_file_as_root(path, codec, decode=True): +def _read_file_as_root(path, open_flag, convert_func): """Read a file as root. :param path Path to the written file. :type path string - :param codec: A codec used to transform the data. - :type codec: StreamCodec + :param open_flag: The flag for opening a file + :type open_flag: string - :param decode: Should the codec decode the data. - :type decode: boolean + :param convert_func: The function for converting data. + :type convert_func: callable """ - with tempfile.NamedTemporaryFile() as fp: + with tempfile.NamedTemporaryFile(open_flag) as fp: copy(path, fp.name, force=True, dereference=True, as_root=True) chmod(fp.name, FileMode.ADD_READ_ALL(), as_root=True) - if decode: - return codec.deserialize(fp.read()) - return codec.serialize(fp.read()) + return convert_func(fp.read()) def write_file(path, data, codec=IdentityCodec(), as_root=False, encode=True): @@ -141,20 +144,24 @@ def write_file(path, data, codec=IdentityCodec(), as_root=False, encode=True): :raises: :class:`UnprocessableEntity` if path not given. """ if path: - if as_root: - _write_file_as_root(path, data, codec, encode=encode) + if encode: + open_flag = 'w' + convert_func = codec.serialize else: - with open(path, 'w') as fp: - if encode: - fp.write(codec.serialize(data)) - else: - fp.write(codec.deserialize(data)) + open_flag = 'wb' + convert_func = codec.deserialize + + if as_root: + _write_file_as_root(path, data, open_flag, convert_func) + else: + with open(path, open_flag) as fp: + fp.write(convert_func(data)) fp.flush() else: raise exception.UnprocessableEntity(_("Invalid path: %s") % path) -def _write_file_as_root(path, data, codec, encode=True): +def _write_file_as_root(path, data, open_flag, convert_func): """Write a file as root. Overwrite any existing contents. :param path Path to the written file. @@ -163,19 +170,16 @@ def _write_file_as_root(path, data, codec, encode=True): :param data: An object representing the file contents. :type data: StreamCodec - :param codec: A codec used to transform the data. - :type codec: StreamCodec + :param open_flag: The flag for opening a file + :type open_flag: string - :param encode: Should the codec encode the data. - :type encode: boolean + :param convert_func: The function for converting data. + :type convert_func: callable """ # The files gets removed automatically once the managing object goes # out of scope. - with tempfile.NamedTemporaryFile('w', delete=False) as fp: - if encode: - fp.write(codec.serialize(data)) - else: - fp.write(codec.deserialize(data)) + with tempfile.NamedTemporaryFile(open_flag, delete=False) as fp: + fp.write(convert_func(data)) fp.flush() fp.close() # Release the resource before proceeding. copy(fp.name, path, force=True, as_root=True) diff --git a/trove/tests/unittests/guestagent/test_operating_system.py b/trove/tests/unittests/guestagent/test_operating_system.py index 9f7969b824..adb66fa6ac 100644 --- a/trove/tests/unittests/guestagent/test_operating_system.py +++ b/trove/tests/unittests/guestagent/test_operating_system.py @@ -18,7 +18,7 @@ import re import stat import tempfile -from mock import call, patch +from mock import call, patch, mock_open from oslo_concurrency.processutils import UnknownArgumentError import six from testtools import ExpectedException @@ -38,8 +38,12 @@ class TestOperatingSystem(trove_testtools.TestCase): def test_base64_codec(self): data = "Line 1\nLine 2\n" - self._test_file_codec(data, Base64Codec()) + # Base64Codec.deserialize returns bytes instead of string. + self._test_file_codec(data, Base64Codec(), + expected_data=data.encode('utf-8')) + # when encoding is reversed for Base64Codec, reading from files + # will call Base64Codec.serialize which returns string. data = "TGluZSAxCkxpbmUgMgo=" self._test_file_codec(data, Base64Codec(), reverse_encoding=True) @@ -192,7 +196,7 @@ class TestOperatingSystem(trove_testtools.TestCase): @patch.object(operating_system, 'copy') def test_write_file_as_root(self, copy_mock): target_file = tempfile.NamedTemporaryFile() - temp_file = tempfile.NamedTemporaryFile() + temp_file = tempfile.NamedTemporaryFile('w') with patch('tempfile.NamedTemporaryFile', return_value=temp_file): operating_system.write_file( @@ -205,13 +209,172 @@ class TestOperatingSystem(trove_testtools.TestCase): side_effect=Exception("Error while executing 'copy'.")) def test_write_file_as_root_with_error(self, copy_mock): target_file = tempfile.NamedTemporaryFile() - temp_file = tempfile.NamedTemporaryFile() + temp_file = tempfile.NamedTemporaryFile('w') with patch('tempfile.NamedTemporaryFile', return_value=temp_file): with ExpectedException(Exception, "Error while executing 'copy'."): operating_system.write_file(target_file.name, "Lorem Ipsum", as_root=True) self.assertFalse(os.path.exists(temp_file.name)) + @patch.object(operating_system, 'exists', return_value=True) + @patch.object(operating_system, 'copy') + @patch.object(operating_system, 'chmod') + @patch.object(IdentityCodec, 'deserialize') + @patch.object(IdentityCodec, 'serialize') + @patch.object(operating_system, 'open', + mock_open(read_data='MockingRead')) + def test_read_file_with_flags_and_conv_func(self, mock_serialize, + mock_deserialize, + mock_chmod, mock_copy, + *args): + test_path = '/path/of/file' + test_data = 'MockingRead' + # use getattr to avoid pylint 'no-member' warning + mock_file = getattr(operating_system, 'open') + + # simple read + operating_system.read_file(test_path) + mock_file.assert_called_once_with(test_path, 'r') + mock_file().read.assert_called_once() + mock_deserialize.called_once_with(test_data) + mock_file.reset_mock() + mock_deserialize.reset_mock() + + # read with decode=False + operating_system.read_file(test_path, decode=False) + mock_file.assert_called_once_with(test_path, 'rb') + mock_file().read.assert_called_once() + mock_serialize.called_once_with(test_data) + mock_file.reset_mock() + mock_serialize.reset_mock() + + # checking _read_file_as_root arguments + with patch.object(operating_system, + '_read_file_as_root') as mock_read_file_as_root: + # simple read as root, + operating_system.read_file(test_path, as_root=True) + mock_read_file_as_root.assert_called_once_with( + test_path, 'r', mock_deserialize) + mock_deserialize.assert_not_called() + mock_read_file_as_root.reset_mock() + + # read as root with decode=False, + operating_system.read_file(test_path, as_root=True, decode=False) + mock_read_file_as_root.assert_called_once_with( + test_path, 'rb', mock_serialize) + mock_serialize.assert_not_called() + + # simple read as root + temp_file = tempfile.NamedTemporaryFile('r') + with patch.object(tempfile, 'NamedTemporaryFile', + return_value=temp_file) as mock_temp_file: + operating_system.read_file(test_path, as_root=True) + mock_temp_file.assert_called_once_with('r') + mock_copy.called_once_with(test_path, temp_file.name, + force=True, dereference=True, + as_root=True) + mock_chmod.called_once_with(temp_file.name, + FileMode.ADD_READ_ALL(), + as_root=True) + mock_deserialize.assert_called_once_with('') + self.assertFalse(os.path.exists(temp_file.name)) + mock_copy.reset_mock() + mock_chmod.reset_mock() + mock_deserialize.reset_mock() + + # read as root with decode=False + temp_file = tempfile.NamedTemporaryFile('rb') + with patch.object(tempfile, 'NamedTemporaryFile', + return_value=temp_file) as mock_temp_file: + operating_system.read_file(test_path, as_root=True, + decode=False) + mock_temp_file.assert_called_once_with('rb') + mock_copy.called_once_with(test_path, temp_file.name, + force=True, dereference=True, + as_root=True) + mock_chmod.called_once_with(temp_file.name, + FileMode.ADD_READ_ALL(), + as_root=True) + mock_serialize.assert_called_once_with(b'') + self.assertFalse(os.path.exists(temp_file.name)) + + @patch.object(operating_system, 'copy') + @patch.object(operating_system, 'chmod') + @patch.object(IdentityCodec, 'deserialize', + return_value=b'DeseiralizedData') + @patch.object(IdentityCodec, 'serialize', + return_value='SerializedData') + @patch.object(operating_system, 'open', mock_open()) + def test_write_file_with_flags_and_conv_func(self, mock_serialize, + mock_deserialize, + mock_chmod, mock_copy): + test_path = '/path/of/file' + test_data = 'MockingWrite' + test_serialize = 'SerializedData' + test_deserialize = b'DeseiralizedData' + mock_file = getattr(operating_system, 'open') + + # simple write + operating_system.write_file(test_path, test_data) + mock_file.assert_called_once_with(test_path, 'w') + mock_serialize.called_once_with(test_data) + mock_file().write.assert_called_once_with(test_serialize) + mock_file().flush.assert_called_once() + mock_file.reset_mock() + mock_serialize.reset_mock() + + # write with encode=False + operating_system.write_file(test_path, test_data, encode=False) + mock_file.assert_called_once_with(test_path, 'wb') + mock_deserialize.called_once_with(test_data) + mock_file().write.assert_called_once_with(test_deserialize) + mock_file().flush.assert_called_once() + mock_file.reset_mock() + mock_deserialize.reset_mock() + + # checking _write_file_as_root arguments + with patch.object(operating_system, + '_write_file_as_root') as mock_write_file_as_root: + # simple write as root, + operating_system.write_file(test_path, test_data, as_root=True) + mock_write_file_as_root.assert_called_once_with( + test_path, test_data, 'w', mock_serialize) + mock_serialize.assert_not_called() + mock_write_file_as_root.reset_mock() + + # read as root with encode=False, + operating_system.write_file(test_path, test_data, + as_root=True, encode=False) + mock_write_file_as_root.assert_called_once_with( + test_path, test_data, 'wb', mock_deserialize) + mock_deserialize.assert_not_called() + + # simple write as root + temp_file = tempfile.NamedTemporaryFile('w') + with patch.object(tempfile, 'NamedTemporaryFile', + return_value=temp_file) as mock_temp_file: + operating_system.write_file(test_path, test_data, as_root=True) + mock_temp_file.assert_called_once_with('w', delete=False) + mock_serialize.assert_called_once_with(test_data) + mock_copy.called_once_with(temp_file.name, test_path, + force=True, as_root=True) + self.assertFalse(os.path.exists(temp_file.name)) + mock_copy.reset_mock() + mock_chmod.reset_mock() + mock_serialize.reset_mock() + + # write as root with decode=False + temp_file = tempfile.NamedTemporaryFile('wb') + with patch.object(tempfile, 'NamedTemporaryFile', + return_value=temp_file) as mock_temp_file: + operating_system.write_file(test_path, test_data, + as_root=True, encode=False) + mock_temp_file.assert_called_once_with('wb', delete=False) + mock_deserialize.assert_called_once_with(test_data) + mock_copy.called_once_with(temp_file.name, test_path, + force=True, as_root=True) + self.assertFalse(os.path.exists(temp_file.name)) + def test_start_service(self): self._assert_service_call(operating_system.start_service, 'cmd_start')