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

@@ -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)
 
-
+    try:
-def _post_config_load(save_tuple):
+        yield
-    sys.path = save_tuple[0]
+    except:
-    for module in set(sys.modules).difference(save_tuple[1]):
+        raise
-        del sys.modules[module]
+    finally:
+        sys.path = old_path
+        for module in set(sys.modules).difference(old_modules):
+            del sys.modules[module]
 
 
 def init():

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):
+    @pytest.fixture(autouse=True)
-        # If we plan to add tests that modify modules themselves, that should
+    def restore_sys_path(self):
-        # be saved as well
+        old_path = sys.path.copy()
-        TestConfigPyModules._old_sys_path = sys.path.copy()
+        yield
+        sys.path = old_path
 
-    def teardown_method(self, method):
+    def test_bind_in_module(self, confpy, qbmodulepy, tmpdir):
-        # Restore path to save the rest of the tests
+        qbmodulepy.write('def run(config):',
-        sys.path = TestConfigPyModules._old_sys_path
+        '    config.bind(",a", "message-info foo", mode="normal")')
-
+        confpy.write_qbmodule()
-    def test_bind_in_module(self, confpy, qbmodulepy):
+        confpy.read()
-        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)
         expected = {'normal': {',a': 'message-info foo'}}
-        assert len(api.errors) == 0
         assert config.instance._values['bindings.commands'] == expected
-
+        assert "qbmodule" not in sys.modules.keys()
-    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 tmpdir not in sys.path
 
-    def test_clear_modules(self, confpy, qbmodulepy):
+    def test_restore_sys_on_err(self, confpy, qbmodulepy, tmpdir):
-        qbmodulepy.write("""def run(config):
+        confpy.write_qbmodule()
-    config.bind(",a", "message-info foo", mode="normal")""")
+        qbmodulepy.write('def run(config):',
-        confpy.write("""import qbmodule
+                         '    1/0')
-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)""")
         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):
+        qbmodulepy.write('def run(config):',
-    pass""")
+                         '    pass')
-        confpy.write("""import foobar
+        confpy.write('import foobar',
-foobar.run(config)""")
+                     '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
+        confpy.write('import sys',
-if sys.path[1:].count(sys.path[0]) != 0:
+                     'if sys.path[0] in sys.path[1:]:',
-    raise Exception('Path not expected')""")
+                     '    raise Exception("Path not expected")')
-        api = configfiles.read_config_py(confpy.filename)
+        confpy.read()
-        assert len(api.errors) == 0
         assert sys.path.count(tmpdir) == 1
 
 
@@ -355,8 +283,8 @@ class TestConfigPy:
 
         """Helper class to get a confpy fixture."""
 
-        def __init__(self, tmpdir):
+        def __init__(self, tmpdir, filename: str = "config.py"):
-            self._confpy = tmpdir / '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)