Restructure/modify environ context manager + tests

Factor out __environ context manager into helper class (and rename).

Move context manager tests into separate test module. Improve tests.

Refactor context manager:
- Change default arg to None
- Simplify: I don't think we need to keep track of new keys;
  environ.clear() + environ.update(old_environ) should be sufficient.
  We significantly reduce complexity at the minor cost of a little more
  space.
- Instead of wrapping in a try/except block, just check if key exists
  before deleting.

Modify test_tool_deps to use the new context manager.

test/unit/tools/test_tool_deps.py

@@ -3,7 +3,6 @@ import tempfile
 from contextlib import contextmanager
 from os import (
     chmod,
-    environ,
     makedirs,
     stat,
     symlink,
@@ -23,6 +22,8 @@ from galaxy.tool_util.deps.resolvers.lmod import LmodDependency, LmodDependencyR
 from galaxy.tool_util.deps.resolvers.modules import ModuleDependency, ModuleDependencyResolver
 from galaxy.util.bunch import Bunch
 
+from .util import modify_environ
+
 # If true, test created DependencyManager objects by serializing out to json and re-constituting.
 ROUND_TRIP_TEST_DEPENDENCY_MANAGER_SERIALIZATION = True
 
@@ -582,7 +583,7 @@ def test_config_modulepath():
 
 def test_config_MODULEPATH():
     # Test reads and splits MODULEPATH if modulepath is not specified.
-    with __environ({"MODULEPATH": "/opt/modules/modulefiles:/usr/local/modules/modulefiles"}):
+    with modify_environ({"MODULEPATH": "/opt/modules/modulefiles:/usr/local/modules/modulefiles"}):
         with __parse_resolvers('''<dependency_resolvers>
   <modules find_by="directory" />
 </dependency_resolvers>
@@ -593,7 +594,7 @@ def test_config_MODULEPATH():
 def test_config_MODULESHOME():
     # Test fallbacks to read MODULESHOME if modulepath is not specified and
     # neither is MODULEPATH.
-    with __environ({"MODULESHOME": "/opt/modules"}, keys_to_remove=["MODULEPATH"]):
+    with modify_environ({"MODULESHOME": "/opt/modules"}, keys_to_remove=["MODULEPATH"]):
         with __parse_resolvers('''<dependency_resolvers>
   <modules find_by="directory" />
 </dependency_resolvers>
@@ -718,72 +719,6 @@ def _first_conda_resolver_options(dm):
     return [r for r in dm.to_dict()["resolvers"] if r["resolver_type"] == "conda"][0]
 
 
-@contextmanager
-def __environ(values, keys_to_remove=[]):
-    """
-    Modify the environment for a test, adding/updating values in dict `values` and
-    removing any environment variables mentioned in list `remove`.
-    """
-    new_keys = set(values.keys()) - set(environ.keys())
-    old_environ = environ.copy()
-    try:
-        environ.update(values)
-        for key in keys_to_remove:
-            try:
-                del environ[key]
-                new_keys.remove(key)  # Because key no longer exists
-            except KeyError:
-                pass
-        yield
-    finally:
-        for key in new_keys:
-            del environ[key]
-        environ.update(old_environ)
-
-
-def test_environ_contextmanager_env_restored():
-    """ os.environ must be preserved across calls to __environ """
-    key, val = 'foo_test_678363', '42'
-    os.environ[key] = val
-    with __environ({}, []):
-        pass
-    assert os.environ[key] == val
-    del os.environ[key]
-
-
-def test_environ_contextmanager_env_updated_and_restored():
-    """
-        os.environ must be preserved across calls to __environ
-        when updating existing or adding new key:value pairs
-    """
-    key, val = 'foo_test_678363', '42'
-    os.environ[key] = val
-    new_key, new_val1 = 'foo_test_new', '53'  # add new key + value
-    new_val2 = '43'  # update old key's value
-    to_update = {new_key: new_val1, key: new_val2}
-    with __environ(values=to_update):
-        pass
-    assert os.environ[key] == val  # value should not change
-    assert new_key not in os.environ  # key should not be added
-    del os.environ[key]
-
-
-def test_environ_contextmanager_env_removed_and_restored():
-    """
-        os.environ must be preserved across calls to __environ
-        when removing key:value pairs
-    """
-    key, val = 'foo_test_678363', '42'
-    os.environ[key] = val
-    new_key, new_val = 'foo_test_new', '53'  # add new key + value
-    to_update = {new_key: new_val}
-    with __environ(values=to_update, keys_to_remove=[key, new_key]):
-        pass
-    assert os.environ[key] == val  # key should be present w/value
-    assert new_key not in os.environ  # key should not be added
-    del os.environ[key]
-
-
 @contextmanager
 def __parse_resolvers(file_content, extension=".xml"):
     with __dependency_manager(file_content, extension=extension) as dm:

test/unit/tools/test_util.py

@@ -0,0 +1,85 @@
+from os import environ
+import pytest
+
+from .util import modify_environ
+
+
+@pytest.fixture
+def load_keyval(request):
+    """
+    Create key/value pair and load it into os.environ. Delete on teardown.
+    """
+    keys = []  # preserve keys for teardown
+
+    def _load_keyval(key='a unique key', val='a value'):
+        # If this is called twice with default values within the same test function,
+        #   it will raise a KeyError. This is intentional: os.environ cannot have duplicate keys.
+        keys.append(key)
+        environ[key] = val
+        return key, val
+
+    def _teardown():
+        for k in keys:
+            del environ[k]
+
+    request.addfinalizer(_teardown)
+    return _load_keyval
+
+
+def test_modify_environ__restore(load_keyval):
+    key, val = load_keyval()
+    with modify_environ({}):
+        assert environ[key] == val  # key/val unchanged
+    assert environ[key] == val  # key/val unchanged
+
+
+def test_modify_environ__add_and_restore(load_keyval):
+    key1, val1 = load_keyval()
+    key2, val2 = 'key to add', 'value to add'
+    to_update = {key2: val2}
+
+    assert key2 not in environ  # ensure key to add does not exist
+    with modify_environ(to_update):
+        assert environ[key1] == val1  # key/val unchanged
+        assert environ[key2] == val2  # new key/val added
+    assert environ[key1] == val1  # key/val unchanged
+    assert key2 not in environ   # new key removed
+
+
+def test_modify_environ__update_and_restore(load_keyval):
+    key1, val1 = load_keyval()
+    key2, val2 = load_keyval('key to update', 'value to update')
+    val2_updated = 'updated'
+    to_update = {key2: val2_updated}
+
+    with modify_environ(to_update):
+        assert environ[key1] == val1  # key/val unchanged
+        assert environ[key2] == val2_updated  # value updated
+    assert environ[key1] == val1  # key/val unchanged
+    assert environ[key2] == val2  # value restored
+
+
+def test_modify_environ__remove_and_restore(load_keyval):
+    key1, val1 = load_keyval()
+    key2, val2 = load_keyval('key to remove', 'value to remove')
+    to_update = {}
+    to_remove = [key2]
+
+    with modify_environ(to_update, to_remove):
+        assert environ[key1] == val1  # key/val unchanged
+        assert key2 not in environ  # key removed
+    assert environ[key1] == val1  # key/val unchanged
+    assert environ[key2] == val2  # key/value restored
+
+
+def test_modify_environ__remove_nonexistant_key(load_keyval):
+    # Test that removing wrong key does not raise an error
+    key1, val1 = load_keyval()
+    key_nonexistant = 'no such key'
+    to_update = {}
+    to_remove = [key_nonexistant]
+
+    assert key_nonexistant not in environ  # ensure key to remove does not exist
+    with modify_environ(to_update, to_remove):
+        assert environ[key1] == val1  # key/val unchanged
+    assert environ[key1] == val1  # key/val unchanged

test/unit/tools/util.py

@@ -0,0 +1,22 @@
+from contextlib import contextmanager
+from os import environ
+
+
+@contextmanager
+def modify_environ(values, keys_to_remove=None):
+    """
+    Modify the environment for a test, adding/updating values in dict `values` and
+    removing any environment variables mentioned in list `keys_to_remove`.
+    """
+    old_environ = environ.copy()
+    try:
+        if values:
+            environ.update(values)
+        if keys_to_remove:
+            for key in keys_to_remove:
+                if key in environ:
+                    del environ[key]
+        yield
+    finally:
+        environ.clear()
+        environ.update(old_environ)