From 4e22b4666d79b6b7f4f9b56aff12d72dae42217d Mon Sep 17 00:00:00 2001 From: Jay Kamat Date: Wed, 20 Sep 2017 21:26:56 -0400 Subject: [PATCH] Convert save-restore of sys to a context-manager Also improve and simplify tests for save/load of sys.module and sys.path --- qutebrowser/config/configfiles.py | 53 +++++------ tests/unit/config/test_configfiles.py | 130 ++++++-------------------- 2 files changed, 54 insertions(+), 129 deletions(-) diff --git a/qutebrowser/config/configfiles.py b/qutebrowser/config/configfiles.py index 91dafe172..561eec0eb 100644 --- a/qutebrowser/config/configfiles.py +++ b/qutebrowser/config/configfiles.py @@ -223,13 +223,6 @@ def read_config_py(filename=None): if not os.path.exists(filename): return api - # Add config directory to python path, so config.py can import other files - # in logical places - old_state = _pre_config_save() - config_dir = os.path.dirname(filename) - if config_dir not in sys.path: - sys.path = [config_dir] + sys.path - container = config.ConfigContainer(config.instance, configapi=api) basename = os.path.basename(filename) @@ -238,20 +231,6 @@ def read_config_py(filename=None): module.c = container module.__file__ = filename - try: - _run_python_config_helper(filename, basename, api, module) - except: - _post_config_load(old_state) - raise - - # Restore previous path, to protect qutebrowser's imports - _post_config_load(old_state) - - api.finalize() - return api - - -def _run_python_config_helper(filename, basename, api, module): try: with open(filename, mode='rb') as f: source = f.read() @@ -268,27 +247,41 @@ def _run_python_config_helper(filename, basename, api, module): raise configexc.ConfigFileErrors(basename, [desc]) except SyntaxError as e: desc = configexc.ConfigErrorDesc("Syntax Error", e, - traceback=traceback.format_exc()) + traceback=traceback.format_exc()) raise configexc.ConfigFileErrors(basename, [desc]) try: - exec(code, module.__dict__) + # Save and restore sys variables + with saved_sys_properties(): + # Add config directory to python path, so config.py can import + # other files in logical places + config_dir = os.path.dirname(filename) + if config_dir not in sys.path: + sys.path = [config_dir] + sys.path + + exec(code, module.__dict__) except Exception as e: api.errors.append(configexc.ConfigErrorDesc( "Unhandled exception", exception=e, traceback=traceback.format_exc())) + api.finalize() + return api -def _pre_config_save(): +@contextlib.contextmanager +def saved_sys_properties(): + """Save various sys properties such as sys.path and sys.modules.""" old_path = sys.path old_modules = sys.modules.copy() - return (old_path, old_modules) - -def _post_config_load(save_tuple): - sys.path = save_tuple[0] - for module in set(sys.modules).difference(save_tuple[1]): - del sys.modules[module] + try: + yield + except: + raise + finally: + sys.path = old_path + for module in set(sys.modules).difference(old_modules): + del sys.modules[module] def init(): diff --git a/tests/unit/config/test_configfiles.py b/tests/unit/config/test_configfiles.py index 0e782efd7..2d8d4cd94 100644 --- a/tests/unit/config/test_configfiles.py +++ b/tests/unit/config/test_configfiles.py @@ -209,121 +209,50 @@ class TestYaml: class TestConfigPyModules: - """Test for ConfigPy Modules.""" - pytestmark = pytest.mark.usefixtures('config_stub', 'key_config_stub') - _old_sys_path = sys.path.copy() - - class ConfPy: - - """Helper class to get a confpy fixture.""" - - def __init__(self, tmpdir): - self._confpy = tmpdir / 'config.py' - self.filename = str(self._confpy) - - def write(self, *lines): - text = '\n'.join(lines) - self._confpy.write_text(text, 'utf-8', ensure=True) - - class QbModulePy: - - """Helper class to get a QbModulePy fixture.""" - - def __init__(self, tmpdir): - self._qbmodulepy = tmpdir / 'qbmodule.py' - self.filename = str(self._qbmodulepy) - - def write(self, *lines): - text = '\n'.join(lines) - self._qbmodulepy.write_text(text, 'utf-8', ensure=True) @pytest.fixture def confpy(self, tmpdir): - return self.ConfPy(tmpdir) + return TestConfigPy.ConfPy(tmpdir) @pytest.fixture def qbmodulepy(self, tmpdir): - return self.QbModulePy(tmpdir) + return TestConfigPy.ConfPy(tmpdir, filename="qbmodule.py") - def setup_method(self, method): - # If we plan to add tests that modify modules themselves, that should - # be saved as well - TestConfigPyModules._old_sys_path = sys.path.copy() + @pytest.fixture(autouse=True) + def restore_sys_path(self): + old_path = sys.path.copy() + yield + sys.path = old_path - def teardown_method(self, method): - # Restore path to save the rest of the tests - sys.path = TestConfigPyModules._old_sys_path - - def test_bind_in_module(self, confpy, qbmodulepy): - qbmodulepy.write("""def run(config): - config.bind(",a", "message-info foo", mode="normal")""") - confpy.write("""import qbmodule -qbmodule.run(config)""") - api = configfiles.read_config_py(confpy.filename) + def test_bind_in_module(self, confpy, qbmodulepy, tmpdir): + qbmodulepy.write('def run(config):', + ' config.bind(",a", "message-info foo", mode="normal")') + confpy.write_qbmodule() + confpy.read() expected = {'normal': {',a': 'message-info foo'}} - assert len(api.errors) == 0 assert config.instance._values['bindings.commands'] == expected - - def test_clear_path(self, confpy, qbmodulepy, tmpdir): - qbmodulepy.write("""def run(config): - config.bind(",a", "message-info foo", mode="normal")""") - confpy.write("""import qbmodule -qbmodule.run(config)""") - api = configfiles.read_config_py(confpy.filename) - assert len(api.errors) == 0 + assert "qbmodule" not in sys.modules.keys() assert tmpdir not in sys.path - def test_clear_modules(self, confpy, qbmodulepy): - qbmodulepy.write("""def run(config): - config.bind(",a", "message-info foo", mode="normal")""") - confpy.write("""import qbmodule -qbmodule.run(config)""") - api = configfiles.read_config_py(confpy.filename) - assert len(api.errors) == 0 - assert "qbmodule" not in sys.modules.keys() - - def test_clear_modules_on_err(self, confpy, qbmodulepy): - qbmodulepy.write("""def run(config): - 1/0""") - confpy.write("""import qbmodule -qbmodule.run(config)""") + def test_restore_sys_on_err(self, confpy, qbmodulepy, tmpdir): + confpy.write_qbmodule() + qbmodulepy.write('def run(config):', + ' 1/0') api = configfiles.read_config_py(confpy.filename) assert len(api.errors) == 1 error = api.errors[0] assert error.text == "Unhandled exception" assert isinstance(error.exception, ZeroDivisionError) - - tblines = error.traceback.strip().splitlines() - assert tblines[0] == "Traceback (most recent call last):" - assert tblines[-1] == "ZeroDivisionError: division by zero" - assert " 1/0" in tblines assert "qbmodule" not in sys.modules.keys() - - def test_clear_path_on_err(self, confpy, qbmodulepy, tmpdir): - qbmodulepy.write("""def run(config): - 1/0""") - confpy.write("""import qbmodule -qbmodule.run(config)""") - api = configfiles.read_config_py(confpy.filename) - - assert len(api.errors) == 1 - error = api.errors[0] - assert error.text == "Unhandled exception" - assert isinstance(error.exception, ZeroDivisionError) - - tblines = error.traceback.strip().splitlines() - assert tblines[0] == "Traceback (most recent call last):" - assert tblines[-1] == "ZeroDivisionError: division by zero" - assert " 1/0" in tblines assert tmpdir not in sys.path def test_fail_on_nonexistent_module(self, confpy, qbmodulepy, tmpdir): - qbmodulepy.write("""def run(config): - pass""") - confpy.write("""import foobar -foobar.run(config)""") + qbmodulepy.write('def run(config):', + ' pass') + confpy.write('import foobar', + 'foobar.run(config)') api = configfiles.read_config_py(confpy.filename) assert len(api.errors) == 1 @@ -337,11 +266,10 @@ foobar.run(config)""") def test_no_double_if_path_exists(self, confpy, qbmodulepy, tmpdir): sys.path.insert(0, tmpdir) - confpy.write("""import sys -if sys.path[1:].count(sys.path[0]) != 0: - raise Exception('Path not expected')""") - api = configfiles.read_config_py(confpy.filename) - assert len(api.errors) == 0 + confpy.write('import sys', + 'if sys.path[0] in sys.path[1:]:', + ' raise Exception("Path not expected")') + confpy.read() assert sys.path.count(tmpdir) == 1 @@ -355,8 +283,8 @@ class TestConfigPy: """Helper class to get a confpy fixture.""" - def __init__(self, tmpdir): - self._confpy = tmpdir / 'config.py' + def __init__(self, tmpdir, filename: str = "config.py"): + self._confpy = tmpdir / filename self.filename = str(self._confpy) def write(self, *lines): @@ -368,6 +296,10 @@ class TestConfigPy: api = configfiles.read_config_py(self.filename) assert not api.errors + def write_qbmodule(self): + self.write('import qbmodule', + 'qbmodule.run(config)') + @pytest.fixture def confpy(self, tmpdir): return self.ConfPy(tmpdir)