Improve tests

pip/configuration.py

@@ -222,7 +222,6 @@ class Configuration(object):
                 # Keeping track of the parsers used
                 self._parsers[variant].append((file_, parser))
 
-    # XXX: This is patched in the tests.
     def _load_file(self, variant, file_):
         logger.debug("For variant '%s', will try loading '%s'", variant, file_)
         parser = self._construct_parser(file_)
@@ -269,6 +268,7 @@ class Configuration(object):
             if key.startswith("PIP_"):
                 yield key[4:].lower(), val
 
+    # XXX: This is patched in the tests.
     def _get_config_files(self):
         """Yields variant and configuration files associated with it.
 

tests/functional/test_configuration.py

@@ -5,7 +5,7 @@ import textwrap
 import pytest
 
 from pip.status_codes import ERROR
-from tests.lib.configuration_helpers import kinds, ConfigurationFileIOMixin
+from tests.lib.configuration_helpers import kinds, ConfigurationMixin
 
 
 def test_no_options_passed_should_error(script):
@@ -13,7 +13,7 @@ def test_no_options_passed_should_error(script):
     assert result.returncode == ERROR
 
 
-class TestBasicLoading(ConfigurationFileIOMixin):
+class TestBasicLoading(ConfigurationMixin):
 
     @pytest.mark.skip("Can't modify underlying file for any mode")
     def test_reads_file_appropriately(self, script):

tests/lib/configuration_helpers.py

@@ -6,7 +6,6 @@ import functools
 import os
 import tempfile
 import textwrap
-from mock import patch
 
 import pip.configuration
 from pip.utils import ensure_dir
@@ -15,41 +14,45 @@ from pip.utils import ensure_dir
 kinds = pip.configuration.kinds
 
 
-class ConfigurationFileIOMixin(object):
+class ConfigurationMixin(object):
 
     def setup(self):
-        self._patches_to_clear = []
+        self.configuration = pip.configuration.Configuration(isolated=False)
+        self._files_to_clear = []
+
+        self._old_environ = os.environ.copy()
 
     def teardown(self):
-        for patch_obj in self._patches_to_clear:
+        for file_ in self._files_to_clear:
-            patch_obj.stop()
+            file_.stop()
+
+        os.environ = self._old_environ
+
+    def patch_configuration(self, variant, di):
+        old = self.configuration._load_config_files
+
+        @functools.wraps(old)
+        def overidden():
+            # Manual Overload
+            self.configuration._config[variant].update(di)
+            self.configuration._parsers[variant].append((None, None))
+            return old()
+
+        self.configuration._load_config_files = overidden
 
     @contextlib.contextmanager
-    def patched_file(self, kind, contents):
+    def tmpfile(self, contents):
         # Create a temporary file
         _, path = tempfile.mkstemp(
-            prefix="pip_", suffix="_" + kind + "config.ini", text=True
+            prefix="pip_", suffix="_config.ini", text=True
         )
 
-        contents = textwrap.dedent(contents)
+        contents = textwrap.dedent(contents).lstrip()
         ensure_dir(os.path.dirname(path))
         with open(path, "w") as f:
             f.write(contents)
 
-        # Convert kind to attribute
+        yield path
-        kind_to_attribute_mapping = {
-            kinds.VENV: "pip.configuration.venv_config_file",
-            kinds.USER: "pip.configuration.new_config_file",
-            kinds.GLOBAL: "pip.configuration.site_config_files",
-        }
-
-        # Patch the attribute
-        # FIXME: This won't work. There's subprocesses!
-        patch_to_make = patch(kind_to_attribute_mapping[kind], path)
-        patch_to_make.start()
-        self._patches_to_clear.append(patch_to_make)
-
-        yield
 
         os.remove(path)
 
@@ -57,21 +60,3 @@ class ConfigurationFileIOMixin(object):
     def get_file_contents(path):
         with open(path) as f:
             return f.read()
-
-
-class ConfigurationPatchingMixin(object):
-
-    def patch_configuration(self, variant, di):
-        old = self.configuration._load_file
-
-        @functools.wraps(old)
-        def overidden(v, file_):
-            if variant == v:
-                self.configuration._config[v].update(di)
-                return object()
-            else:
-                return old(v, file_)
-        self.configuration._load_file = overidden
-
-    def setup(self):
-        self.configuration = pip.configuration.Configuration(isolated=False)

tests/unit/test_configuration.py

@@ -2,99 +2,138 @@
 """
 
 import os
-import tempfile
-import textwrap
 from mock import MagicMock
 
 from pip.locations import venv_config_file, new_config_file, site_config_files
 from pip.exceptions import ConfigurationError
 
-from tests.lib.configuration_helpers import kinds, ConfigurationPatchingMixin
+from tests.lib.configuration_helpers import kinds, ConfigurationMixin
 
 
-class TestConfigurationLoading(ConfigurationPatchingMixin):
+class TestConfigurationLoading(ConfigurationMixin):
 
     def test_global_loading(self):
         self.patch_configuration(kinds.GLOBAL, {"test.hello": "1"})
+
         self.configuration.load()
         assert self.configuration.get_value("test.hello") == "1"
 
     def test_user_loading(self):
         self.patch_configuration(kinds.USER, {"test.hello": "2"})
+
         self.configuration.load()
         assert self.configuration.get_value("test.hello") == "2"
 
     def test_venv_loading(self):
         self.patch_configuration(kinds.VENV, {"test.hello": "3"})
+
         self.configuration.load()
         assert self.configuration.get_value("test.hello") == "3"
 
     def test_environment_config_loading(self):
-        contents = textwrap.dedent("""\
+        contents = """
             [test]
             hello = 4
-        """)
+        """
 
-        _, config_file = tempfile.mkstemp('-pip.cfg', 'test-')
+        with self.tmpfile(contents) as config_file:
-        os.environ["PIP_CONFIG_FILE"] = config_file
+            os.environ["PIP_CONFIG_FILE"] = config_file
-        with open(config_file, "w") as f:
+
-            f.write(contents)
+            self.configuration.load()
+            assert self.configuration.get_value("test.hello") == "4", \
+                self.configuration._config
+
+    def test_environment_var_loading(self):
+        os.environ["PIP_HELLO"] = "5"
 
         self.configuration.load()
-        assert self.configuration.get_value("test.hello") == "4"
+        assert self.configuration.get_value(":env:.hello") == "5"
 
 
-class TestConfigurationPrecedence(ConfigurationPatchingMixin):
+class TestConfigurationPrecedence(ConfigurationMixin):
     # Tests for methods to that determine the order of precedence of
     # configuration options
 
-    def test_global_overriden_by_user(self):
+    def test_env_overides_venv(self):
-        self.patch_configuration(kinds.GLOBAL, {"test.hello": "1"})
+        self.patch_configuration(kinds.VENV, {"test.hello": "1"})
+        self.patch_configuration(kinds.ENV, {"test.hello": "0"})
+        self.configuration.load()
+
+        assert self.configuration.get_value("test.hello") == "0"
+
+    def test_env_overides_user(self):
         self.patch_configuration(kinds.USER, {"test.hello": "2"})
+        self.patch_configuration(kinds.ENV, {"test.hello": "0"})
         self.configuration.load()
 
-        assert self.configuration.get_value("test.hello") == "2"
+        assert self.configuration.get_value("test.hello") == "0"
 
-    def test_global_overriden_by_venv(self):
+    def test_env_overides_global(self):
-        self.patch_configuration(kinds.GLOBAL, {"test.hello": "1"})
+        self.patch_configuration(kinds.GLOBAL, {"test.hello": "3"})
-        self.patch_configuration(kinds.VENV, {"test.hello": "3"})
+        self.patch_configuration(kinds.ENV, {"test.hello": "0"})
         self.configuration.load()
 
-        assert self.configuration.get_value("test.hello") == "3"
+        assert self.configuration.get_value("test.hello") == "0"
 
-    def test_user_overriden_by_venv(self):
+    def test_venv_overides_user(self):
         self.patch_configuration(kinds.USER, {"test.hello": "2"})
-        self.patch_configuration(kinds.VENV, {"test.hello": "3"})
+        self.patch_configuration(kinds.VENV, {"test.hello": "1"})
-        self.configuration.load()
-
-        assert self.configuration.get_value("test.hello") == "3"
-
-    def test_global_not_overriden_by_environment_var(self):
-        self.patch_configuration(kinds.GLOBAL, {"test.hello": "1"})
-        os.environ["PIP_HELLO"] = "4"
         self.configuration.load()
 
         assert self.configuration.get_value("test.hello") == "1"
-        assert self.configuration.get_value(":env:.hello") == "4"
 
-    def test_user_not_overriden_by_environment_var(self):
+    def test_venv_overides_global(self):
+        self.patch_configuration(kinds.GLOBAL, {"test.hello": "3"})
+        self.patch_configuration(kinds.VENV, {"test.hello": "1"})
+        self.configuration.load()
+
+        assert self.configuration.get_value("test.hello") == "1"
+
+    def test_user_overides_global(self):
+        self.patch_configuration(kinds.GLOBAL, {"test.hello": "3"})
         self.patch_configuration(kinds.USER, {"test.hello": "2"})
-        os.environ["PIP_HELLO"] = "4"
         self.configuration.load()
 
         assert self.configuration.get_value("test.hello") == "2"
-        assert self.configuration.get_value(":env:.hello") == "4"
+
+    def test_env_not_overriden_by_environment_var(self):
+        self.patch_configuration(kinds.ENV, {"test.hello": "1"})
+        os.environ["PIP_HELLO"] = "5"
+
+        self.configuration.load()
+
+        assert self.configuration.get_value("test.hello") == "1"
+        assert self.configuration.get_value(":env:.hello") == "5"
 
     def test_venv_not_overriden_by_environment_var(self):
-        self.patch_configuration(kinds.VENV, {"test.hello": "3"})
+        self.patch_configuration(kinds.VENV, {"test.hello": "2"})
-        os.environ["PIP_HELLO"] = "4"
+        os.environ["PIP_HELLO"] = "5"
+
+        self.configuration.load()
+
+        assert self.configuration.get_value("test.hello") == "2"
+        assert self.configuration.get_value(":env:.hello") == "5"
+
+    def test_user_not_overriden_by_environment_var(self):
+        self.patch_configuration(kinds.USER, {"test.hello": "3"})
+        os.environ["PIP_HELLO"] = "5"
+
         self.configuration.load()
 
         assert self.configuration.get_value("test.hello") == "3"
-        assert self.configuration.get_value(":env:.hello") == "4"
+        assert self.configuration.get_value(":env:.hello") == "5"
+
+    def test_global_not_overriden_by_environment_var(self):
+        self.patch_configuration(kinds.GLOBAL, {"test.hello": "4"})
+        os.environ["PIP_HELLO"] = "5"
+
+        self.configuration.load()
+
+        assert self.configuration.get_value("test.hello") == "4"
+        assert self.configuration.get_value(":env:.hello") == "5"
 
 
-class TestConfigurationModification(ConfigurationPatchingMixin):
+class TestConfigurationModification(ConfigurationMixin):
     # Tests for methods to that modify the state of a Configuration
 
     def test_no_specific_given_modification(self):