From 6e78cc6847dd6bb72ebcabd361ba6b159d3fb9a6 Mon Sep 17 00:00:00 2001 From: Sergey Golitsynskiy Date: Mon, 8 Jul 2019 17:59:25 +0200 Subject: [PATCH 1/5] Fix bug in a unit test There was a bug in a context manager inside a unit test. This caused nondeterministic behavior of some unit tests after a refactoring. Bug fixed; added 3 unit tests to test the context manager. --- test/unit/tools/test_tool_deps.py | 56 +++++++++++++++++++++++++++---- 1 file changed, 50 insertions(+), 6 deletions(-) diff --git a/test/unit/tools/test_tool_deps.py b/test/unit/tools/test_tool_deps.py index c28818bd526..1a4efa6c09a 100644 --- a/test/unit/tools/test_tool_deps.py +++ b/test/unit/tools/test_tool_deps.py @@ -593,7 +593,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"}, remove="MODULEPATH"): + with __environ({"MODULESHOME": "/opt/modules"}, keys_to_remove=["MODULEPATH"]): with __parse_resolvers(''' @@ -719,25 +719,69 @@ def _first_conda_resolver_options(dm): @contextmanager -def __environ(values, remove=[]): +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(environ.keys()) - set(values.keys()) + new_keys = set(values.keys()) - set(environ.keys()) old_environ = environ.copy() try: environ.update(values) - for to_remove in remove: + for key in keys_to_remove: try: - del environ[remove] + del environ[key] + new_keys.remove(key) # Because key no longer exists except KeyError: pass yield finally: - environ.update(old_environ) 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 From 73cd69a13fdb72866b02ab0d988f9d04b9d81461 Mon Sep 17 00:00:00 2001 From: Sergey Golitsynskiy Date: Tue, 9 Jul 2019 00:39:21 +0200 Subject: [PATCH 2/5] Fix lint errors --- test/unit/tools/test_tool_deps.py | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/test/unit/tools/test_tool_deps.py b/test/unit/tools/test_tool_deps.py index 1a4efa6c09a..c8f3c5366e9 100644 --- a/test/unit/tools/test_tool_deps.py +++ b/test/unit/tools/test_tool_deps.py @@ -731,7 +731,7 @@ def __environ(values, keys_to_remove=[]): for key in keys_to_remove: try: del environ[key] - new_keys.remove(key) # Because key no longer exists + new_keys.remove(key) # Because key no longer exists except KeyError: pass yield @@ -752,35 +752,35 @@ def test_environ_contextmanager_env_restored(): def test_environ_contextmanager_env_updated_and_restored(): - """ - os.environ must be preserved across calls to __environ + """ + 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 } + 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 + 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 + """ + 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 } + 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 + 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] From 3d59d94aa8ce04c7be3ce71fcf00500c429a6674 Mon Sep 17 00:00:00 2001 From: Sergey Golitsynskiy Date: Sat, 13 Jul 2019 13:08:59 -0400 Subject: [PATCH 3/5] 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 | 73 ++------------------------ test/unit/tools/test_util.py | 85 +++++++++++++++++++++++++++++++ test/unit/tools/util.py | 22 ++++++++ 3 files changed, 111 insertions(+), 69 deletions(-) create mode 100644 test/unit/tools/test_util.py create mode 100644 test/unit/tools/util.py diff --git a/test/unit/tools/test_tool_deps.py b/test/unit/tools/test_tool_deps.py index c8f3c5366e9..11f9413158e 100644 --- a/test/unit/tools/test_tool_deps.py +++ b/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(''' @@ -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(''' @@ -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: diff --git a/test/unit/tools/test_util.py b/test/unit/tools/test_util.py new file mode 100644 index 00000000000..7962469ce58 --- /dev/null +++ b/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 diff --git a/test/unit/tools/util.py b/test/unit/tools/util.py new file mode 100644 index 00000000000..2bc0340711c --- /dev/null +++ b/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) From f3f979fd5db13f52233ea0ddb95eb163e3fb55af Mon Sep 17 00:00:00 2001 From: Sergey Golitsynskiy Date: Sat, 13 Jul 2019 19:28:31 -0400 Subject: [PATCH 4/5] Fix linting errors (import stms) --- test/unit/tools/test_tool_deps.py | 1 - test/unit/tools/test_util.py | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/tools/test_tool_deps.py b/test/unit/tools/test_tool_deps.py index 11f9413158e..1f15d880a0c 100644 --- a/test/unit/tools/test_tool_deps.py +++ b/test/unit/tools/test_tool_deps.py @@ -21,7 +21,6 @@ from galaxy.tool_util.deps.resolvers.galaxy_packages import GalaxyPackageDepende from galaxy.tool_util.deps.resolvers.lmod import LmodDependency, LmodDependencyResolver 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. diff --git a/test/unit/tools/test_util.py b/test/unit/tools/test_util.py index 7962469ce58..e2778fbf74e 100644 --- a/test/unit/tools/test_util.py +++ b/test/unit/tools/test_util.py @@ -1,4 +1,5 @@ from os import environ + import pytest from .util import modify_environ From 3df3ff0bec16d1174914da92eb547965c5ebd255 Mon Sep 17 00:00:00 2001 From: Sergey Golitsynskiy Date: Sat, 13 Jul 2019 20:18:57 -0400 Subject: [PATCH 5/5] Fix package test The cause of this is related to pulling tool_util out of tools: https://github.com/galaxyproject/galaxy/pull/8279 The solution here is a temporary hack (add helper module, util.py, to tests/, then make tests a package). A better solution will be to separate test/unit/tools into tools/ and tool_util/ - I intend to take care of it soon. --- packages/tool_util/tests/__init__.py | 0 packages/tool_util/tests/util.py | 1 + 2 files changed, 1 insertion(+) create mode 100644 packages/tool_util/tests/__init__.py create mode 120000 packages/tool_util/tests/util.py diff --git a/packages/tool_util/tests/__init__.py b/packages/tool_util/tests/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/packages/tool_util/tests/util.py b/packages/tool_util/tests/util.py new file mode 120000 index 00000000000..9469bc59223 --- /dev/null +++ b/packages/tool_util/tests/util.py @@ -0,0 +1 @@ +../../../test/unit/tools/util.py \ No newline at end of file