February 2017 - tor-commits - lists.torproject.org

[stem/master] Integ tests that should only run once weren't suppressed
by atagar＠torproject.org 12 Feb '17

12 Feb '17

commit 402360c78745db05ea4dfeb57148bf1e689ea0cd Author: Damian Johnson <atagar(a)torproject.org> Date: Sun Feb 12 11:16:16 2017 -0800 Integ tests that should only run once weren't suppressed Huh. Why did I include a 'self' reference in RAN_TESTS? Isn't needed and in fact made our conditional always evaluate to false, breaking '@only_run_once'. This caused our integ tests to run much longer than necessary when using targets like 'RUN_ALL' (such as tor's testing target and jenkins). This should also fix our integ tests with the RUN_ALL target. They were failing because our first run of the installation tests clean up the temporary resources made for it, causing all the following invocations to fail. --- test/runner.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/test/runner.py b/test/runner.py index ff1da66..8ab65f6 100644 --- a/test/runner.py +++ b/test/runner.py @@ -91,7 +91,6 @@ Torrc = stem.util.enum.Enum( ('PTRACE', 'DisableDebuggerAttachment 0'), ) -# (test_instance, test_name) tuples that we've registered as having been ran RAN_TESTS = [] @@ -173,8 +172,8 @@ def only_run_once(func): """ def wrapped(self, *args, **kwargs): - if (self, self.id()) not in RAN_TESTS: - RAN_TESTS.append((self, self.id())) + if self.id() not in RAN_TESTS: + RAN_TESTS.append(self.id()) return func(self, *args, **kwargs) else: skip(self, '(already ran)')

1 0

[stem/master] Fix test_setconf_event test when CONF_CHANGED event comes first
by atagar＠torproject.org 12 Feb '17

12 Feb '17

commit e6cd021cd428ecbd0cd8adbf0c070b84c125aff7 Author: Damian Johnson <atagar(a)torproject.org> Date: Thu Feb 2 10:48:51 2017 -0800 Fix test_setconf_event test when CONF_CHANGED event comes first Oops, our earlier fix to work regardless of the order actually broke the test if CONF_CHANGED comes in first. Trouble is that we check for an int rather than a string status code. --- test/integ/socket/control_message.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integ/socket/control_message.py b/test/integ/socket/control_message.py index 47f77c1..5f6ff42 100644 --- a/test/integ/socket/control_message.py +++ b/test/integ/socket/control_message.py @@ -166,7 +166,7 @@ class TestControlMessage(unittest.TestCase): msg1 = control_socket.recv() msg2 = control_socket.recv() - if msg1.content()[0][0] == 650: + if msg1.content()[0][0] == '650': conf_changed_event, setconf_response = msg1, msg2 else: setconf_response, conf_changed_event = msg1, msg2

1 0

[stem/master] Avoid parsing malformed lines multiple times
by atagar＠torproject.org 12 Feb '17

12 Feb '17

commit 8397c43c30e24d12dc01a84f1b2fc51999506f8b Author: Damian Johnson <atagar(a)torproject.org> Date: Thu Feb 2 10:58:21 2017 -0800 Avoid parsing malformed lines multiple times Descriptor lines usually set multiple attributes. When we've read a line for one of those attributes there's no point in reading it again for another. This has no noticeable impact I can tell on performance (multiple calls to a variety of related attributes in a malformed descriptor is very much an edge case). Just doing this because when it happens it's clearly wasted work. --- stem/descriptor/__init__.py | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/stem/descriptor/__init__.py b/stem/descriptor/__init__.py index 8c31b86..7c64293 100644 --- a/stem/descriptor/__init__.py +++ b/stem/descriptor/__init__.py @@ -646,9 +646,9 @@ class Descriptor(object): # Doing so works since it stops recursing after several dozen iterations # (not sure why), but horrible in terms of performance. - def has_attr(): + def has_attr(attr): try: - super(Descriptor, self).__getattribute__(name) + super(Descriptor, self).__getattribute__(attr) return True except: return False @@ -658,18 +658,20 @@ class Descriptor(object): # a. we still need to lazy load this # b. we read the whole descriptor but it wasn't present, so needs the default - if name in self.ATTRIBUTES and not has_attr(): + if name in self.ATTRIBUTES and not has_attr(name): default, parsing_function = self.ATTRIBUTES[name] if self._lazy_loading: try: parsing_function(self, self._entries) except (ValueError, KeyError): - # Despite having a validation failure we might've set something. If - # not then set the default. + # Set defaults for anything the parsing function should've covered. + # Despite having a validation failure some attributes might be set in + # which case we keep them. - if not has_attr(): - setattr(self, name, _copy(default)) + for attr_name, (attr_default, attr_parser) in self.ATTRIBUTES.items(): + if parsing_function == attr_parser and not has_attr(attr_name): + setattr(self, attr_name, _copy(attr_default)) else: setattr(self, name, _copy(default))

1 0

[stem/master] Speed up remaining unit tests
by atagar＠torproject.org 12 Feb '17

12 Feb '17

commit c6e1fa22a2615bed4282c26c7fb618e7702720a1 Author: Damian Johnson <atagar(a)torproject.org> Date: Wed Feb 8 10:08:31 2017 -0800 Speed up remaining unit tests Attempted to get all our individual tests under 100ms. Wasn't always successful, but close. Biggest benefit was omitting unnecessary iterations from the authentication test where we exercise all use cases. This dropped it from 1.1s to 0.4s. On my wimpy little netbook this improves the overall unit test runtime from 7.5s to 6.4s (15% faster). --- test/unit/connection/authentication.py | 59 ++++++++++++----------- test/unit/descriptor/networkstatus/document_v3.py | 13 ++--- test/unit/exit_policy/policy.py | 2 +- test/unit/manual.py | 14 +++++- 4 files changed, 48 insertions(+), 40 deletions(-) diff --git a/test/unit/connection/authentication.py b/test/unit/connection/authentication.py index 70de6d7..b7ccc0d 100644 --- a/test/unit/connection/authentication.py +++ b/test/unit/connection/authentication.py @@ -93,16 +93,22 @@ class TestAuthenticate(unittest.TestCase): stem.connection.AuthMethod.UNKNOWN, ], include_empty = True) - for protocolinfo_auth_methods in auth_method_combinations: - # protocolinfo input for the authenticate() call we'll be making - protocolinfo_arg = mocking.get_protocolinfo_response( - auth_methods = protocolinfo_auth_methods, - cookie_path = '/tmp/blah', - ) + protocolinfo = mocking.get_protocolinfo_response(cookie_path = '/tmp/blah') + for auth_methods in auth_method_combinations: for auth_none_exc in all_auth_none_exc: for auth_password_exc in all_auth_password_exc: for auth_cookie_exc in all_auth_cookie_exc: + # Skip iteration if it's to test exceptions for authentication + # we're not using. + + if auth_none_exc and stem.connection.AuthMethod.NONE not in auth_methods: + continue + elif auth_password_exc and stem.connection.AuthMethod.PASSWORD not in auth_methods: + continue + elif auth_cookie_exc and stem.connection.AuthMethod.COOKIE not in auth_methods and stem.connection.AuthMethod.SAFECOOKIE not in auth_methods: + continue + # Determine if the authenticate() call will succeed and mock each # of the authenticate_* function to raise its given exception. # @@ -116,33 +122,28 @@ class TestAuthenticate(unittest.TestCase): # matters so the above inaccuracies seem fine. expect_success = False - auth_mocks = { - stem.connection.AuthMethod.NONE: - (authenticate_none_mock, auth_none_exc), - stem.connection.AuthMethod.PASSWORD: - (authenticate_password_mock, auth_password_exc), - stem.connection.AuthMethod.COOKIE: - (authenticate_cookie_mock, auth_cookie_exc), - stem.connection.AuthMethod.SAFECOOKIE: - (authenticate_safecookie_mock, auth_cookie_exc), - } - - for auth_method in auth_mocks: - auth_mock, raised_exc = auth_mocks[auth_method] - - if not raised_exc: - # Mocking this authentication method so it will succeed. If - # it's among the protocolinfo methods then expect success. - - auth_mock.side_effect = None - expect_success |= auth_method in protocolinfo_auth_methods - else: + protocolinfo.auth_methods = auth_methods + + for auth_method in auth_methods: + if auth_method == stem.connection.AuthMethod.NONE: + auth_mock, raised_exc = authenticate_none_mock, auth_none_exc + elif auth_method == stem.connection.AuthMethod.PASSWORD: + auth_mock, raised_exc = authenticate_password_mock, auth_password_exc + elif auth_method == stem.connection.AuthMethod.COOKIE: + auth_mock, raised_exc = authenticate_cookie_mock, auth_cookie_exc + elif auth_method == stem.connection.AuthMethod.SAFECOOKIE: + auth_mock, raised_exc = authenticate_safecookie_mock, auth_cookie_exc + + if raised_exc: auth_mock.side_effect = raised_exc + else: + auth_mock.side_effect = None + expect_success = True if expect_success: - stem.connection.authenticate(None, 'blah', None, protocolinfo_arg) + stem.connection.authenticate(None, 'blah', None, protocolinfo) else: - self.assertRaises(stem.connection.AuthenticationFailure, stem.connection.authenticate, None, 'blah', None, protocolinfo_arg) + self.assertRaises(stem.connection.AuthenticationFailure, stem.connection.authenticate, None, 'blah', None, protocolinfo) # revert logging back to normal stem_logger.setLevel(log.logging_level(log.TRACE)) diff --git a/test/unit/descriptor/networkstatus/document_v3.py b/test/unit/descriptor/networkstatus/document_v3.py index a3da8a4..e6e5d76 100644 --- a/test/unit/descriptor/networkstatus/document_v3.py +++ b/test/unit/descriptor/networkstatus/document_v3.py @@ -662,15 +662,12 @@ DnN5aFtYKiTc19qIC7Nmo+afPdDEf0MlJvEOP5EWl3w= '2012-12-12 01:a1:01', ) - for field in ('published', 'valid-after', 'fresh-until', 'valid-until'): - attr = field.replace('-', '_') - - for test_value in test_values: - content = get_network_status_document_v3({'vote-status': 'vote', field: test_value}, content = True) - self.assertRaises(ValueError, NetworkStatusDocumentV3, content, True) + for test_value in test_values: + content = get_network_status_document_v3({'vote-status': 'vote', 'published': test_value}, content = True) + self.assertRaises(ValueError, NetworkStatusDocumentV3, content, True) - document = NetworkStatusDocumentV3(content, False) - self.assertEqual(None, getattr(document, attr)) + document = NetworkStatusDocumentV3(content, False) + self.assertEqual(None, document.published) def test_voting_delay(self): """ diff --git a/test/unit/exit_policy/policy.py b/test/unit/exit_policy/policy.py index b1187ac..29e3837 100644 --- a/test/unit/exit_policy/policy.py +++ b/test/unit/exit_policy/policy.py @@ -64,7 +64,7 @@ class TestExitPolicy(unittest.TestCase): policy = ExitPolicy('accept *:80', 'accept *:443', 'reject *:*') - for index in range(1, 500): + for index in range(1, 100): ip_addr = '%i.%i.%i.%i' % (index / 2, index / 2, index / 2, index / 2) expected_result = index in (80, 443) diff --git a/test/unit/manual.py b/test/unit/manual.py index 65957c8..8338af7 100644 --- a/test/unit/manual.py +++ b/test/unit/manual.py @@ -103,6 +103,16 @@ EXPECTED_CONFIG_OPTIONS['MaxAdvertisedBandwidth'] = stem.manual.ConfigOption( summary = 'Limit for the bandwidth we advertise as being available for relaying', description = 'If set, we will not advertise more than this amount of bandwidth for our BandwidthRate. Server operators who want to reduce the number of clients who ask to build circuits through them (since this is proportional to advertised bandwidth rate) can thus reduce the CPU demands on their server without impacting network performance.') +CACHED_MANUAL = None + +def _cached_manual(): + global CACHED_MANUAL + + if CACHED_MANUAL is None: + CACHED_MANUAL = stem.manual.Manual.from_cache() + + return CACHED_MANUAL + class TestManual(unittest.TestCase): def test_has_all_summaries(self): @@ -112,7 +122,7 @@ class TestManual(unittest.TestCase): to write a little summary. They're located in 'stem/settings.cfg'. """ - manual = stem.manual.Manual.from_cache() + manual = _cached_manual() present = set(manual.config_options.keys()) expected = set([key[15:] for key in stem.manual._config(lowercase = False) if key.startswith('manual.summary.')]) @@ -212,7 +222,7 @@ class TestManual(unittest.TestCase): self.assertEqual(manual, loaded_manual) def test_cached_manual(self): - manual = stem.manual.Manual.from_cache() + manual = _cached_manual() self.assertEqual('tor - The second-generation onion router', manual.name) self.assertEqual('tor [OPTION value]...', manual.synopsis)

1 0

[stem/master] Reduce remaining sleeps
by atagar＠torproject.org 12 Feb '17

12 Feb '17

commit 3bdc1d1a9363a1c039f0ea47a1f93a2a8de864b3 Author: Damian Johnson <atagar(a)torproject.org> Date: Wed Feb 8 10:29:36 2017 -0800 Reduce remaining sleeps Easy drop of our integ runtime from 1-3s simply by reducing the sleeps we do (they can be arbitrarily short, our tests don't care). --- test/integ/control/base_controller.py | 2 +- test/integ/control/controller.py | 2 +- test/integ/process.py | 12 ++++++++---- test/integ/socket/control_socket.py | 4 ++-- test/integ/util/system.py | 4 ++-- 5 files changed, 14 insertions(+), 10 deletions(-) diff --git a/test/integ/control/base_controller.py b/test/integ/control/base_controller.py index 51576a8..dc2e478 100644 --- a/test/integ/control/base_controller.py +++ b/test/integ/control/base_controller.py @@ -242,7 +242,7 @@ class TestBaseController(unittest.TestCase): controller.add_status_listener(state_observer.listener, True) controller.close() - time.sleep(0.1) # not much work going on so this doesn't need to be much + time.sleep(0.001) # not much work going on so this doesn't need to be much self.assertEqual(controller, state_observer.controller) self.assertEqual(stem.control.State.CLOSED, state_observer.state) self.assertTrue(state_observer.timestamp <= time.time()) diff --git a/test/integ/control/controller.py b/test/integ/control/controller.py index 0d17b0b..a1f155b 100644 --- a/test/integ/control/controller.py +++ b/test/integ/control/controller.py @@ -111,7 +111,7 @@ class TestController(unittest.TestCase): if (time.time() - before) > 2: self.fail("We've waited a couple seconds for SIGHUP to generate an event, but it didn't come") - time.sleep(0.1) + time.sleep(0.001) after = time.time() diff --git a/test/integ/process.py b/test/integ/process.py index b082d0b..19d6750 100644 --- a/test/integ/process.py +++ b/test/integ/process.py @@ -421,11 +421,13 @@ class TestProcess(unittest.TestCase): # # https://trac.torproject.org/projects/tor/ticket/21281 - for seconds_waited in range(30): + start_time = time.time() + + while time.time() - start_time < 30: if tor_process.poll() == 0: return # tor exited - time.sleep(1) + time.sleep(0.01) self.fail("tor didn't quit after the process that owned it terminated") @@ -455,11 +457,13 @@ class TestProcess(unittest.TestCase): controller.close() # give tor a few seconds to quit - for seconds_waited in range(5): + start_time = time.time() + + while time.time() - start_time < 5: if tor_process.poll() == 0: return # tor exited - time.sleep(1) + time.sleep(0.01) self.fail("tor didn't quit after the controller that owned it disconnected") diff --git a/test/integ/socket/control_socket.py b/test/integ/socket/control_socket.py index 83851be..a1e603b 100644 --- a/test/integ/socket/control_socket.py +++ b/test/integ/socket/control_socket.py @@ -38,7 +38,7 @@ class TestControlSocket(unittest.TestCase): # connection time should be absolute (shouldn't change as time goes on) - time.sleep(0.1) + time.sleep(0.001) self.assertEqual(connection_time, control_socket.connection_time()) # should change to the disconnection time if we detactch @@ -49,7 +49,7 @@ class TestControlSocket(unittest.TestCase): # then change again if we reconnect - time.sleep(0.1) + time.sleep(0.001) control_socket.connect() reconnection_time = control_socket.connection_time() self.assertTrue(disconnection_time < reconnection_time <= time.time()) diff --git a/test/integ/util/system.py b/test/integ/util/system.py index 7933492..dd301f9 100644 --- a/test/integ/util/system.py +++ b/test/integ/util/system.py @@ -569,8 +569,8 @@ class TestSystem(unittest.TestCase): """ initial = stem.util.system.SYSTEM_CALL_TIME - stem.util.system.call('sleep 0.5') - self.assertTrue(stem.util.system.SYSTEM_CALL_TIME - initial > 0.4) + stem.util.system.call('sleep 0.005') + self.assertTrue(stem.util.system.SYSTEM_CALL_TIME - initial > 0.004) def test_set_process_name(self): """

1 0

[stem/master] Rearrange installation integ tests
by atagar＠torproject.org 12 Feb '17

12 Feb '17

commit 7ec72f1fdaedcd791fc98904b0ad4f7cdb0c22b6 Author: Damian Johnson <atagar(a)torproject.org> Date: Wed Feb 8 13:28:48 2017 -0800 Rearrange installation integ tests Rearranging our installation tests to be based around the two setup.py commands we run (install and sdist). We were hiding a substantial chunk of runtime by doing the bulk of our work in a setUpClass clause. This is unnessary and reads better this way. --- test/integ/installation.py | 157 +++++++++++++++++++++------------------------ 1 file changed, 73 insertions(+), 84 deletions(-) diff --git a/test/integ/installation.py b/test/integ/installation.py index 43ba3d8..d1a1eb8 100644 --- a/test/integ/installation.py +++ b/test/integ/installation.py @@ -14,61 +14,93 @@ import test.util INSTALL_MISMATCH_MSG = "Running 'python setup.py sdist' doesn't match our git contents in the following way. The manifest in our setup.py may need to be updated...\n\n" -class TestInstallation(unittest.TestCase): - # TODO: remove when dropping support for python 2.6 - skip_reason = 'setUpClass() unsupported in python 2.6' +def _install(path): + """ + Installs to the given location. + + :param str path: location to install ourselves to - @classmethod - def setUpClass(self): - self.site_packages_path = None - self.skip_reason = None - self.installation_error = None + :returns: **str** where our site packages are located - if not os.path.exists(os.path.join(test.util.STEM_BASE, 'setup.py')): - self.skip_reason = '(only for git checkout)' + :raises: **AssertionError** if installation is unsuccessful + """ - original_cwd = os.getcwd() + original_cwd = os.getcwd() - try: - os.chdir(test.util.STEM_BASE) - stem.util.system.call(sys.executable + ' setup.py install --prefix /tmp/stem_test') - stem.util.system.call(sys.executable + ' setup.py clean --all') # tidy up the build directory - site_packages_paths = glob.glob('/tmp/stem_test/lib*/*/site-packages') + try: + os.chdir(test.util.STEM_BASE) + stem.util.system.call('%s setup.py install --prefix %s' % (sys.executable, path)) + stem.util.system.call('%s setup.py clean --all' % sys.executable) # tidy up the build directory + site_packages_paths = glob.glob('%s/lib*/*/site-packages' % path) - if len(site_packages_paths) != 1: - self.installation_error = 'We should only have a single site-packages directory, but instead had: %s' % site_packages_paths + if len(site_packages_paths) != 1: + raise AssertionError('We should only have a single site-packages directory, but instead had: %s' % site_packages_paths) - self.site_packages_path = site_packages_paths[0] - except Exception as exc: - self.installation_error = 'Unable to download the man page: %s' % exc - finally: - os.chdir(original_cwd) + return site_packages_paths[0] + except Exception as exc: + raise AssertionError("Unable to install with 'python setup.py install': %s" % exc) + finally: + os.chdir(original_cwd) + + +def _assert_has_all_files(path): + """ + Check that all the files in the stem directory are present in the + installation. This is a very common gotcha since our setup.py + requires us to remember to add new modules and non-source files. + + :raises: **AssertionError** files don't match our content + """ + + expected, installed = set(), set() + + for root, dirnames, filenames in os.walk(os.path.join(test.util.STEM_BASE, 'stem')): + for filename in filenames: + file_format = filename.split('.')[-1] - @classmethod - def tearDownClass(self): - if os.path.exists('/tmp/stem_test'): - shutil.rmtree('/tmp/stem_test') + if file_format not in test.util.IGNORED_FILE_TYPES: + expected.add(os.path.join(root, filename)[len(test.util.STEM_BASE) + 1:]) - def requires_installation(self): - if self.skip_reason: - test.runner.skip(self, self.skip_reason) - return True - elif self.installation_error: - self.fail(self.installation_error) + for root, dirnames, filenames in os.walk(path): + for filename in filenames: + if not filename.endswith('.pyc') and not filename.endswith('egg-info'): + installed.add(os.path.join(root, filename)[len(path) + 1:]) - return False + missing = expected.difference(installed) + extra = installed.difference(expected) + if missing: + raise AssertionError("The following files were expected to be in our installation but weren't. Maybe our setup.py needs to be updated?\n\n%s" % '\n'.join(missing)) + elif extra: + raise AssertionError("The following files weren't expected to be in our installation.\n\n%s" % '\n'.join(extra)) + + +class TestInstallation(unittest.TestCase): @test.runner.only_run_once - def test_sdist_matches_git(self): + def test_install(self): """ - Check the source distribution tarball we make for releases matches the - contents of 'git archive'. This primarily is meant to test that our - MANIFEST.in is up to date. + Installs with 'python setup.py install' and checks we can use what we + install. """ - if self.requires_installation(): - return - elif not stem.util.system.is_available('git'): + try: + site_packages_path = _install('/tmp/stem_test') + + self.assertEqual(stem.__version__, stem.util.system.call([sys.executable, '-c', "import sys;sys.path.insert(0, '%s');import stem;print(stem.__version__)" % site_packages_path])[0]) + _assert_has_all_files(site_packages_path) + finally: + if os.path.exists('/tmp/stem_test'): + shutil.rmtree('/tmp/stem_test') + + @test.runner.only_run_once + def test_sdist(self): + """ + Creates a source distribution tarball with 'python setup.py sdist' and + checks that it matches the content of our git repository. This primarily is + meant to test that our MANIFEST.in is up to date. + """ + + if not stem.util.system.is_available('git'): test.runner.skip(self, '(git unavailable)') return @@ -102,46 +134,3 @@ class TestInstallation(unittest.TestCase): if issues: self.fail(INSTALL_MISMATCH_MSG + '\n'.join(issues)) - - @test.runner.only_run_once - def test_installing_stem(self): - """ - Attempt to use the package we install. - """ - - if self.requires_installation(): - return - - self.assertEqual(stem.__version__, stem.util.system.call([sys.executable, '-c', "import sys;sys.path.insert(0, '%s');import stem;print(stem.__version__)" % self.site_packages_path])[0]) - - def test_installs_all_files(self): - """ - Check that all the files in the stem directory are present in the - installation. This is a very common gotcha since our setup.py - requires us to remember to add new modules and non-source files. - """ - - if self.requires_installation(): - return - - expected, installed = set(), set() - - for root, dirnames, filenames in os.walk(os.path.join(test.util.STEM_BASE, 'stem')): - for filename in filenames: - file_format = filename.split('.')[-1] - - if file_format not in test.util.IGNORED_FILE_TYPES: - expected.add(os.path.join(root, filename)[len(test.util.STEM_BASE) + 1:]) - - for root, dirnames, filenames in os.walk(self.site_packages_path): - for filename in filenames: - if not filename.endswith('.pyc') and not filename.endswith('egg-info'): - installed.add(os.path.join(root, filename)[len(self.site_packages_path) + 1:]) - - missing = expected.difference(installed) - extra = installed.difference(expected) - - if missing: - self.fail("The following files were expected to be in our installation but weren't. Maybe our setup.py needs to be updated?\n\n%s" % '\n'.join(missing)) - elif extra: - self.fail("The following files weren't expected to be in our installation.\n\n%s" % '\n'.join(extra))

1 0

[stem/master] Speed up installation integ tests
by atagar＠torproject.org 12 Feb '17

12 Feb '17

commit b3a145677e12af8df22cdbf27d53d1babba1ac62 Author: Damian Johnson <atagar(a)torproject.org> Date: Wed Feb 8 17:17:30 2017 -0800 Speed up installation integ tests Installing and creating source distributions take several seconds, making these slow tests. Thankfully this time is all disk io so we can simply kick off another thread to parallelize this with other tests. Shaves whole seconds off our integ test runtime (... which for our tests is quite a bit!). --- run_tests.py | 4 +++ test/integ/installation.py | 78 +++++++++++++++++++++++++++++----------------- 2 files changed, 54 insertions(+), 28 deletions(-) diff --git a/run_tests.py b/run_tests.py index abb5123..3db6280 100755 --- a/run_tests.py +++ b/run_tests.py @@ -26,6 +26,7 @@ import stem.util.system import stem.util.test_tools import test.arguments +import test.integ.installation import test.output import test.runner import test.util @@ -213,6 +214,9 @@ def main(): our_version = stem.version.get_system_tor_version(args.tor_path) skipped_targets = [] + if not args.specific_test or 'test.integ.installation'.startswith(args.specific_test): + test.integ.installation.setup() + for target in args.run_targets: # check if we meet this target's tor version prerequisites diff --git a/test/integ/installation.py b/test/integ/installation.py index d1a1eb8..cfe771a 100644 --- a/test/integ/installation.py +++ b/test/integ/installation.py @@ -3,6 +3,7 @@ import os import shutil import sys import tarfile +import threading import unittest import stem @@ -13,34 +14,50 @@ import test.util INSTALL_MISMATCH_MSG = "Running 'python setup.py sdist' doesn't match our git contents in the following way. The manifest in our setup.py may need to be updated...\n\n" +BASE_INSTALL_PATH = '/tmp/stem_test' +SETUP_THREAD, INSTALL_FAILURE, INSTALL_PATH, SDIST_FAILURE = None, None, None, None -def _install(path): + +def setup(): + """ + Performs setup our tests will need. This mostly just needs disk iops so it + can happen asynchronously with other tests. """ - Installs to the given location. - - :param str path: location to install ourselves to - :returns: **str** where our site packages are located + global SETUP_THREAD - :raises: **AssertionError** if installation is unsuccessful - """ + def _setup(): + global INSTALL_FAILURE, INSTALL_PATH, SDIST_FAILURE + original_cwd = os.getcwd() + + try: + os.chdir(test.util.STEM_BASE) + + try: + os.chdir(test.util.STEM_BASE) + stem.util.system.call('%s setup.py install --prefix %s' % (sys.executable, BASE_INSTALL_PATH)) + stem.util.system.call('%s setup.py clean --all' % sys.executable) # tidy up the build directory + site_packages_paths = glob.glob('%s/lib*/*/site-packages' % BASE_INSTALL_PATH) - original_cwd = os.getcwd() + if len(site_packages_paths) != 1: + raise AssertionError('We should only have a single site-packages directory, but instead had: %s' % site_packages_paths) - try: - os.chdir(test.util.STEM_BASE) - stem.util.system.call('%s setup.py install --prefix %s' % (sys.executable, path)) - stem.util.system.call('%s setup.py clean --all' % sys.executable) # tidy up the build directory - site_packages_paths = glob.glob('%s/lib*/*/site-packages' % path) + INSTALL_PATH = site_packages_paths[0] + except Exception as exc: + INSTALL_FAILURE = AssertionError("Unable to install with 'python setup.py install': %s" % exc) - if len(site_packages_paths) != 1: - raise AssertionError('We should only have a single site-packages directory, but instead had: %s' % site_packages_paths) + try: + stem.util.system.call('%s setup.py sdist' % sys.executable) + except Exception as exc: + SDIST_FAILURE = exc + finally: + os.chdir(original_cwd) - return site_packages_paths[0] - except Exception as exc: - raise AssertionError("Unable to install with 'python setup.py install': %s" % exc) - finally: - os.chdir(original_cwd) + if SETUP_THREAD is None: + SETUP_THREAD = threading.Thread(target = _setup) + SETUP_THREAD.start() + + return SETUP_THREAD def _assert_has_all_files(path): @@ -84,13 +101,16 @@ class TestInstallation(unittest.TestCase): """ try: - site_packages_path = _install('/tmp/stem_test') + setup().join() + + if INSTALL_FAILURE: + raise INSTALL_FAILURE - self.assertEqual(stem.__version__, stem.util.system.call([sys.executable, '-c', "import sys;sys.path.insert(0, '%s');import stem;print(stem.__version__)" % site_packages_path])[0]) - _assert_has_all_files(site_packages_path) + self.assertEqual(stem.__version__, stem.util.system.call([sys.executable, '-c', "import sys;sys.path.insert(0, '%s');import stem;print(stem.__version__)" % INSTALL_PATH])[0]) + _assert_has_all_files(INSTALL_PATH) finally: - if os.path.exists('/tmp/stem_test'): - shutil.rmtree('/tmp/stem_test') + if os.path.exists(BASE_INSTALL_PATH): + shutil.rmtree(BASE_INSTALL_PATH) @test.runner.only_run_once def test_sdist(self): @@ -104,14 +124,16 @@ class TestInstallation(unittest.TestCase): test.runner.skip(self, '(git unavailable)') return + setup().join() + + if SDIST_FAILURE: + raise SDIST_FAILURE + original_cwd = os.getcwd() dist_path = os.path.join(test.util.STEM_BASE, 'dist') git_contents = [line.split()[-1] for line in stem.util.system.call('git ls-tree --full-tree -r HEAD')] try: - os.chdir(test.util.STEM_BASE) - stem.util.system.call(sys.executable + ' setup.py sdist') - # tarball has a prefix 'stem-[verion]' directory so stipping that out dist_tar = tarfile.open(os.path.join(dist_path, 'stem-dry-run-%s.tar.gz' % stem.__version__))

1 0

[stem/master] More testing improvements
by atagar＠torproject.org 12 Feb '17

12 Feb '17

commit 7b227419e3340633d276976033aac721764e09d7 Merge: 783061d f3785a1 Author: Damian Johnson <atagar(a)torproject.org> Date: Sat Feb 11 21:29:02 2017 -0800 More testing improvements Handful of small additional testing improvements. For my netbook this makes a fairly noticeable different but on my pc improvements are modest... unit: 2.4s => 2.1s (12% faster) integ: 46.4s => 39.4s (15% faster) docs/change_log.rst | 1 + run_tests.py | 11 ++ stem/descriptor/__init__.py | 16 +- stem/util/system.py | 40 ++++- test/integ/control/base_controller.py | 2 +- test/integ/control/controller.py | 2 +- test/integ/installation.py | 192 ++++++++++++---------- test/integ/process.py | 12 +- test/integ/socket/control_message.py | 2 +- test/integ/socket/control_socket.py | 4 +- test/integ/util/system.py | 11 +- test/unit/connection/authentication.py | 59 +++---- test/unit/descriptor/networkstatus/document_v3.py | 13 +- test/unit/exit_policy/policy.py | 2 +- test/unit/manual.py | 15 +- 15 files changed, 231 insertions(+), 151 deletions(-)

1 0

[stem/master] Add timeout argument to stem.util.system.call()
by atagar＠torproject.org 12 Feb '17

12 Feb '17

commit f3785a139eaecf4ff896315e92b89588ae3e2c9c Author: Damian Johnson <atagar(a)torproject.org> Date: Thu Feb 9 10:35:47 2017 -0800 Add timeout argument to stem.util.system.call() While making our installer integ test more reliable I've had it take over a minute or even fail to finish at all several times. Allowing timeouts of our system calls is generally useful anyway so good excuse to make this addition. --- docs/change_log.rst | 1 + run_tests.py | 9 +++++++- stem/util/system.py | 40 +++++++++++++++++++++++++++++++++--- test/integ/installation.py | 51 +++++++++++++++++++++++----------------------- test/integ/util/system.py | 7 +++++++ test/unit/manual.py | 1 + 6 files changed, 80 insertions(+), 29 deletions(-) diff --git a/docs/change_log.rst b/docs/change_log.rst index 85619b2..338150c 100644 --- a/docs/change_log.rst +++ b/docs/change_log.rst @@ -55,6 +55,7 @@ The following are only available within Stem's `git repository * **Utilities** + * Added timeout argument to :func:`~stem.util.system.call` * Added :class:`~stem.util.test_tools.TimedTestRunner` and :func:`~stem.util.test_tools.test_runtimes` .. _version_1.5: diff --git a/run_tests.py b/run_tests.py index 3db6280..501cbfa 100755 --- a/run_tests.py +++ b/run_tests.py @@ -213,9 +213,10 @@ def main(): our_version = stem.version.get_system_tor_version(args.tor_path) skipped_targets = [] + integ_setup_thread = None if not args.specific_test or 'test.integ.installation'.startswith(args.specific_test): - test.integ.installation.setup() + integ_setup_thread = test.integ.installation.setup() for target in args.run_targets: # check if we meet this target's tor version prerequisites @@ -247,6 +248,9 @@ def main(): # We should have joined on all threads. If not then that indicates a # leak that could both likely be a bug and disrupt further targets. + if integ_setup_thread: + integ_setup_thread.join() + active_threads = threading.enumerate() if len(active_threads) > 1: @@ -267,6 +271,9 @@ def main(): except OSError: error_tracker.register_error() finally: + if integ_setup_thread: + test.integ.installation.clean() + println() integ_runner.stop() println() diff --git a/stem/util/system.py b/stem/util/system.py index c1db6d0..4951058 100644 --- a/stem/util/system.py +++ b/stem/util/system.py @@ -166,6 +166,20 @@ class CallError(OSError): return self.msg +class CallTimeoutError(CallError): + """ + Error response when making a system call that has timed out. + + .. versionadded:: 1.6.0 + + :var float timeout: time we waited + """ + + def __init__(self, msg, command, exit_status, runtime, stdout, stderr, timeout): + super(CallTimeoutError, self).__init__(msg, command, exit_status, runtime, stdout, stderr) + self.timeout = timeout + + def is_windows(): """ Checks if we are running on Windows. @@ -1020,7 +1034,7 @@ def files_with_suffix(base_path, suffix): yield os.path.join(root, filename) -def call(command, default = UNDEFINED, ignore_exit_status = False, env = None): +def call(command, default = UNDEFINED, ignore_exit_status = False, timeout = None, env = None): """ call(command, default = UNDEFINED, ignore_exit_status = False) @@ -1035,16 +1049,22 @@ def call(command, default = UNDEFINED, ignore_exit_status = False, env = None): .. versionchanged:: 1.5.0 Added env argument. + .. versionchanged:: 1.6.0 + Added timeout argument. + :param str,list command: command to be issued :param object default: response if the query fails :param bool ignore_exit_status: reports failure if our command's exit status was non-zero + :param float timeout: maximum seconds to wait, blocks indefinitely if + **None** :param dict env: environment variables :returns: **list** with the lines of output from the command - :raises: **CallError** if this fails and no default was provided, this is an - **OSError** subclass + :raises: + * **CallError** if this fails and no default was provided + * **CallTimeoutError** if the timeout is reached without a default """ global SYSTEM_CALL_TIME @@ -1062,6 +1082,13 @@ def call(command, default = UNDEFINED, ignore_exit_status = False, env = None): process = subprocess.Popen(command_list, stdout = subprocess.PIPE, stderr = subprocess.PIPE, shell = is_shell_command, env = env) + if timeout: + while process.poll() is None: + if time.time() - start_time > timeout: + raise CallTimeoutError("Process didn't finish after %0.1f seconds" % timeout, ' '.join(command_list), None, timeout, '', '', timeout) + + time.sleep(0.001) + stdout, stderr = process.communicate() stdout, stderr = stdout.strip(), stderr.strip() runtime = time.time() - start_time @@ -1085,6 +1112,13 @@ def call(command, default = UNDEFINED, ignore_exit_status = False, env = None): return stdout.decode('utf-8', 'replace').splitlines() else: return [] + except CallTimeoutError as exc: + log.debug('System call (timeout): %s (after %0.4fs)' % (command, timeout)) + + if default != UNDEFINED: + return default + else: + raise except OSError as exc: log.debug('System call (failed): %s (error: %s)' % (command, exc)) diff --git a/test/integ/installation.py b/test/integ/installation.py index cfe771a..2048ea3 100644 --- a/test/integ/installation.py +++ b/test/integ/installation.py @@ -15,6 +15,7 @@ import test.util INSTALL_MISMATCH_MSG = "Running 'python setup.py sdist' doesn't match our git contents in the following way. The manifest in our setup.py may need to be updated...\n\n" BASE_INSTALL_PATH = '/tmp/stem_test' +DIST_PATH = os.path.join(test.util.STEM_BASE, 'dist') SETUP_THREAD, INSTALL_FAILURE, INSTALL_PATH, SDIST_FAILURE = None, None, None, None @@ -35,8 +36,8 @@ def setup(): try: os.chdir(test.util.STEM_BASE) - stem.util.system.call('%s setup.py install --prefix %s' % (sys.executable, BASE_INSTALL_PATH)) - stem.util.system.call('%s setup.py clean --all' % sys.executable) # tidy up the build directory + stem.util.system.call('%s setup.py install --prefix %s' % (sys.executable, BASE_INSTALL_PATH), timeout = 60) + stem.util.system.call('%s setup.py clean --all' % sys.executable, timeout = 60) # tidy up the build directory site_packages_paths = glob.glob('%s/lib*/*/site-packages' % BASE_INSTALL_PATH) if len(site_packages_paths) != 1: @@ -46,10 +47,13 @@ def setup(): except Exception as exc: INSTALL_FAILURE = AssertionError("Unable to install with 'python setup.py install': %s" % exc) - try: - stem.util.system.call('%s setup.py sdist' % sys.executable) - except Exception as exc: - SDIST_FAILURE = exc + if not os.path.exists(DIST_PATH): + try: + stem.util.system.call('%s setup.py sdist' % sys.executable, timeout = 60) + except Exception as exc: + SDIST_FAILURE = exc + else: + SDIST_FAILURE = AssertionError("%s already exists, maybe you manually ran 'python setup.py sdist'?" % DIST_PATH) finally: os.chdir(original_cwd) @@ -60,6 +64,14 @@ def setup(): return SETUP_THREAD +def clean(): + if os.path.exists(BASE_INSTALL_PATH): + shutil.rmtree(BASE_INSTALL_PATH) + + if os.path.exists(DIST_PATH): + shutil.rmtree(DIST_PATH) + + def _assert_has_all_files(path): """ Check that all the files in the stem directory are present in the @@ -100,17 +112,14 @@ class TestInstallation(unittest.TestCase): install. """ - try: + if not INSTALL_PATH: setup().join() - if INSTALL_FAILURE: - raise INSTALL_FAILURE + if INSTALL_FAILURE: + raise INSTALL_FAILURE - self.assertEqual(stem.__version__, stem.util.system.call([sys.executable, '-c', "import sys;sys.path.insert(0, '%s');import stem;print(stem.__version__)" % INSTALL_PATH])[0]) - _assert_has_all_files(INSTALL_PATH) - finally: - if os.path.exists(BASE_INSTALL_PATH): - shutil.rmtree(BASE_INSTALL_PATH) + self.assertEqual(stem.__version__, stem.util.system.call([sys.executable, '-c', "import sys;sys.path.insert(0, '%s');import stem;print(stem.__version__)" % INSTALL_PATH])[0]) + _assert_has_all_files(INSTALL_PATH) @test.runner.only_run_once def test_sdist(self): @@ -129,20 +138,12 @@ class TestInstallation(unittest.TestCase): if SDIST_FAILURE: raise SDIST_FAILURE - original_cwd = os.getcwd() - dist_path = os.path.join(test.util.STEM_BASE, 'dist') git_contents = [line.split()[-1] for line in stem.util.system.call('git ls-tree --full-tree -r HEAD')] - try: - # tarball has a prefix 'stem-[verion]' directory so stipping that out - - dist_tar = tarfile.open(os.path.join(dist_path, 'stem-dry-run-%s.tar.gz' % stem.__version__)) - tar_contents = ['/'.join(info.name.split('/')[1:]) for info in dist_tar.getmembers() if info.isfile()] - finally: - if os.path.exists(dist_path): - shutil.rmtree(dist_path) + # tarball has a prefix 'stem-[verion]' directory so stipping that out - os.chdir(original_cwd) + dist_tar = tarfile.open(os.path.join(DIST_PATH, 'stem-dry-run-%s.tar.gz' % stem.__version__)) + tar_contents = ['/'.join(info.name.split('/')[1:]) for info in dist_tar.getmembers() if info.isfile()] issues = [] diff --git a/test/integ/util/system.py b/test/integ/util/system.py index dd301f9..b6e66a0 100644 --- a/test/integ/util/system.py +++ b/test/integ/util/system.py @@ -563,6 +563,13 @@ class TestSystem(unittest.TestCase): self.assertEqual(home_dir, stem.util.system.expand_path('~%s' % username)) self.assertEqual(os.path.join(home_dir, 'foo'), stem.util.system.expand_path('~%s/foo' % username)) + def test_call_timeout(self): + try: + stem.util.system.call('sleep 1', timeout = 0.001) + self.fail("sleep should've timed out") + except stem.util.system.CallTimeoutError as exc: + self.assertEqual("Process didn't finish after 0.0 seconds", str(exc)) + def test_call_time_tracked(self): """ Check that time taken in the call() function is tracked by SYSTEM_CALL_TIME. diff --git a/test/unit/manual.py b/test/unit/manual.py index 8338af7..5f939c3 100644 --- a/test/unit/manual.py +++ b/test/unit/manual.py @@ -105,6 +105,7 @@ EXPECTED_CONFIG_OPTIONS['MaxAdvertisedBandwidth'] = stem.manual.ConfigOption( CACHED_MANUAL = None + def _cached_manual(): global CACHED_MANUAL

1 0

[stem/master] Cookie paths with wide characters couldn't be loaded
by atagar＠torproject.org 12 Feb '17

12 Feb '17

commit 48f76996c3bfa50580568acfebd4af838c7f7d9e Author: Damian Johnson <atagar(a)torproject.org> Date: Sat Feb 11 15:24:57 2017 -0800 Cookie paths with wide characters couldn't be loaded When using an authentication cookie with a path containing wide characters (chinese, japanese, etc) we failed to unescape and decode them, causing authentication to fail. Troubleshooting this revealed that we actually were incorrectly escaping all responses. Tor's esc_for_log covers more than the limited map we had... https://gitweb.torproject.org/tor.git/tree/src/common/util.c#n1241 As such using python's function for doing C-style unescaping. Fingers crossed these always match up. ;P --- docs/change_log.rst | 1 + stem/response/__init__.py | 85 ++++++++++++++++---------------------- stem/response/protocolinfo.py | 10 ++++- test/unit/response/protocolinfo.py | 17 ++++++++ 4 files changed, 63 insertions(+), 50 deletions(-) diff --git a/docs/change_log.rst b/docs/change_log.rst index 338150c..edd5fa8 100644 --- a/docs/change_log.rst +++ b/docs/change_log.rst @@ -46,6 +46,7 @@ The following are only available within Stem's `git repository * **Controller** * Added the GUARD_WAIT :data:`~stem.CircStatus` (:spec:`6446210`) + * Unable to use cookie auth when path includes wide characters (chinese, japanese, etc) * **Descriptors** diff --git a/stem/response/__init__.py b/stem/response/__init__.py index 8d1eaf8..a84654c 100644 --- a/stem/response/__init__.py +++ b/stem/response/__init__.py @@ -30,11 +30,13 @@ Parses replies from the control socket. +- pop_mapping - removes and returns the next entry as a KEY=VALUE mapping """ +import codecs import io import re import threading import stem.socket +import stem.util.str_tools __all__ = [ 'add_onion', @@ -51,16 +53,6 @@ __all__ = [ KEY_ARG = re.compile('^(\S+)=') -# Escape sequences from the 'esc_for_log' function of tor's 'common/util.c'. -# It's hard to tell what controller functions use this in practice, but direct -# users are... -# - 'COOKIEFILE' field of PROTOCOLINFO responses -# - logged messages about bugs -# - the 'getinfo_helper_listeners' function of control.c - -CONTROL_ESCAPES = {r'\\': '\\', r'\"': '\"', r'\'': '\'', - r'\r': '\r', r'\n': '\n', r'\t': '\t'} - def convert(response_type, message, **kwargs): """ @@ -333,7 +325,7 @@ class ControlLine(str): """ Checks if our next entry is a quoted value or not. - :param bool escaped: unescapes the CONTROL_ESCAPES escape sequences + :param bool escaped: unescapes the string :returns: **True** if the next entry can be parsed as a quoted value, **False** otherwise """ @@ -347,7 +339,7 @@ class ControlLine(str): :param str key: checks that the key matches this value, skipping the check if **None** :param bool quoted: checks that the mapping is to a quoted value - :param bool escaped: unescapes the CONTROL_ESCAPES escape sequences + :param bool escaped: unescapes the string :returns: **True** if the next entry can be parsed as a key=value mapping, **False** otherwise @@ -405,7 +397,7 @@ class ControlLine(str): "this has a \\" and \\\\ in it" :param bool quoted: parses the next entry as a quoted value, removing the quotes - :param bool escaped: unescapes the CONTROL_ESCAPES escape sequences + :param bool escaped: unescapes the string :returns: **str** of the next space separated entry @@ -415,17 +407,21 @@ class ControlLine(str): """ with self._remainder_lock: - next_entry, remainder = _parse_entry(self._remainder, quoted, escaped) + next_entry, remainder = _parse_entry(self._remainder, quoted, escaped, False) self._remainder = remainder return next_entry - def pop_mapping(self, quoted = False, escaped = False): + def pop_mapping(self, quoted = False, escaped = False, get_bytes = False): """ Parses the next space separated entry as a KEY=VALUE mapping, removing it and the space from our remaining content. + .. versionchanged:: 1.6.0 + Added the get_bytes argument. + :param bool quoted: parses the value as being quoted, removing the quotes - :param bool escaped: unescapes the CONTROL_ESCAPES escape sequences + :param bool escaped: unescapes the string + :param bool get_bytes: provides **bytes** for the **value** rather than a **str** :returns: **tuple** of the form (key, value) @@ -447,18 +443,18 @@ class ControlLine(str): key = key_match.groups()[0] remainder = self._remainder[key_match.end():] - next_entry, remainder = _parse_entry(remainder, quoted, escaped) + next_entry, remainder = _parse_entry(remainder, quoted, escaped, get_bytes) self._remainder = remainder return (key, next_entry) -def _parse_entry(line, quoted, escaped): +def _parse_entry(line, quoted, escaped, get_bytes): """ Parses the next entry from the given space separated content. :param str line: content to be parsed :param bool quoted: parses the next entry as a quoted value, removing the quotes - :param bool escaped: unescapes the CONTROL_ESCAPES escape sequences + :param bool escaped: unescapes the string :returns: **tuple** of the form (entry, remainder) @@ -488,7 +484,26 @@ def _parse_entry(line, quoted, escaped): next_entry, remainder = remainder, '' if escaped: - next_entry = _unescape(next_entry) + # Tor does escaping in its 'esc_for_log' function of 'common/util.c'. It's + # hard to tell what controller functions use this in practice, but direct + # users are... + # + # * 'COOKIEFILE' field of PROTOCOLINFO responses + # * logged messages about bugs + # * the 'getinfo_helper_listeners' function of control.c + # + # Ideally we'd use "next_entry.decode('string_escape')" but it was removed + # in python 3.x and 'unicode_escape' isn't quite the same... + # + # https://stackoverflow.com/questions/14820429/how-do-i-decodestring-escape-i… + + next_entry = codecs.escape_decode(next_entry)[0] + + if stem.prereq.is_python_3() and not get_bytes: + next_entry = stem.util.str_tools._to_unicode(next_entry) # normalize back to str + + if get_bytes: + next_entry = stem.util.str_tools._to_bytes(next_entry) return (next_entry, remainder.lstrip()) @@ -498,7 +513,7 @@ def _get_quote_indices(line, escaped): Provides the indices of the next two quotes in the given content. :param str line: content to be parsed - :param bool escaped: unescapes the CONTROL_ESCAPES escape sequences + :param bool escaped: unescapes the string :returns: **tuple** of two ints, indices being -1 if a quote doesn't exist """ @@ -519,34 +534,6 @@ def _get_quote_indices(line, escaped): return tuple(indices) -def _unescape(entry): - # Unescapes the given string with the mappings in CONTROL_ESCAPES. - # - # This can't be a simple series of str.replace() calls because replacements - # need to be excluded from consideration for further unescaping. For - # instance, '\\t' should be converted to '\t' rather than a tab. - - def _pop_with_unescape(entry): - # Pop either the first character or the escape sequence conversion the - # entry starts with. This provides a tuple of... - # - # (unescaped prefix, remaining entry) - - for esc_sequence, replacement in CONTROL_ESCAPES.items(): - if entry.startswith(esc_sequence): - return (replacement, entry[len(esc_sequence):]) - - return (entry[0], entry[1:]) - - result = [] - - while entry: - prefix, entry = _pop_with_unescape(entry) - result.append(prefix) - - return ''.join(result) - - class SingleLineResponse(ControlMessage): """ Reply to a request that performs an action rather than querying data. These diff --git a/stem/response/protocolinfo.py b/stem/response/protocolinfo.py index 6243b4d..a4320b8 100644 --- a/stem/response/protocolinfo.py +++ b/stem/response/protocolinfo.py @@ -1,9 +1,13 @@ # Copyright 2012-2017, Damian Johnson and The Tor Project # See LICENSE for licensing information +import sys + +import stem.prereq import stem.response import stem.socket import stem.version +import stem.util.str_tools from stem.connection import AuthMethod from stem.util import log @@ -101,8 +105,12 @@ class ProtocolInfoResponse(stem.response.ControlMessage): auth_methods.append(AuthMethod.UNKNOWN) # parse optional COOKIEFILE mapping (quoted and can have escapes) + if line.is_next_mapping('COOKIEFILE', True, True): - self.cookie_path = line.pop_mapping(True, True)[1] + self.cookie_path = line.pop_mapping(True, True, get_bytes = True)[1].decode(sys.getfilesystemencoding()) + + if stem.prereq.is_python_3(): + self.cookie_path = stem.util.str_tools._to_unicode(self.cookie_path) # normalize back to str elif line_type == 'VERSION': # Line format: # VersionLine = "250-VERSION" SP "Tor=" TorVersion OptArguments CRLF diff --git a/test/unit/response/protocolinfo.py b/test/unit/response/protocolinfo.py index 4dcf291..c95076a 100644 --- a/test/unit/response/protocolinfo.py +++ b/test/unit/response/protocolinfo.py @@ -49,11 +49,18 @@ UNKNOWN_AUTH = """250-PROTOCOLINFO 1 MINIMUM_RESPONSE = """250-PROTOCOLINFO 5 250 OK""" +UNICODE_COOKIE_PATH = r"""250-PROTOCOLINFO 1 +250-AUTH METHODS=COOKIE COOKIEFILE="/home/user/\346\226\207\346\241\243/tor-browser_en-US/Browser/TorBrowser/Data/Tor/control_auth_cookie" +250-VERSION Tor="0.2.1.30" +250 OK""" + RELATIVE_COOKIE_PATH = r"""250-PROTOCOLINFO 1 250-AUTH METHODS=COOKIE COOKIEFILE="./tor-browser_en-US/Data/control_auth_cookie" 250-VERSION Tor="0.2.1.30" 250 OK""" +EXPECTED_UNICODE_PATH = b"/home/user/\346\226\207\346\241\243/tor-browser_en-US/Browser/TorBrowser/Data/Tor/control_auth_cookie".decode('utf-8') + class TestProtocolInfoResponse(unittest.TestCase): def test_convert(self): @@ -151,6 +158,16 @@ class TestProtocolInfoResponse(unittest.TestCase): self.assertEqual((), control_message.unknown_auth_methods) self.assertEqual(None, control_message.cookie_path) + @patch('sys.getfilesystemencoding', Mock(return_value = 'UTF-8')) + def test_unicode_cookie(self): + """ + Checks an authentication cookie with a unicode path. + """ + + control_message = mocking.get_message(UNICODE_COOKIE_PATH) + stem.response.convert('PROTOCOLINFO', control_message) + self.assertEqual(EXPECTED_UNICODE_PATH, control_message.cookie_path) + @patch('stem.util.proc.is_available', Mock(return_value = False)) @patch('stem.util.system.is_available', Mock(return_value = True)) def test_relative_cookie(self):

1 0