From 212c5433099769d6077c5b48b085ef174b8d7550 Mon Sep 17 00:00:00 2001 From: Rafael Sarmiento Date: Wed, 19 Jan 2022 11:13:54 +0100 Subject: [PATCH 1/4] add hash to module name --- reframe/utility/__init__.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/reframe/utility/__init__.py b/reframe/utility/__init__.py index e4fb29c398..a1cff4cccd 100644 --- a/reframe/utility/__init__.py +++ b/reframe/utility/__init__.py @@ -20,6 +20,7 @@ import reframe from collections import UserDict +from hashlib import sha256 from . import typecheck as typ @@ -54,7 +55,9 @@ def _get_module_name(filename): def _do_import_module_from_file(filename, module_name=None): + module_hash = sha256(filename.encode('utf-8')).hexdigest()[:8] module_name = module_name or _get_module_name(filename) + module_name = f'{module_name}_{module_hash}' if module_name in sys.modules: return sys.modules[module_name] From 391dcd37f67ad35544c5ccd48c784ffa5b21d8e6 Mon Sep 17 00:00:00 2001 From: Rafael Sarmiento Date: Wed, 19 Jan 2022 15:24:04 +0100 Subject: [PATCH 2/4] ad option for hash and fix unit tests --- reframe/utility/__init__.py | 12 +++++++----- unittests/test_utility.py | 12 ++++++++++++ 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/reframe/utility/__init__.py b/reframe/utility/__init__.py index a1cff4cccd..e64ba1cd46 100644 --- a/reframe/utility/__init__.py +++ b/reframe/utility/__init__.py @@ -54,10 +54,12 @@ def _get_module_name(filename): return barename.replace(os.sep, '.') -def _do_import_module_from_file(filename, module_name=None): - module_hash = sha256(filename.encode('utf-8')).hexdigest()[:8] +def _do_import_module_from_file(filename, module_name=None, unique_name=False): module_name = module_name or _get_module_name(filename) - module_name = f'{module_name}_{module_hash}' + if unique_name: + module_hash = sha256(filename.encode('utf-8')).hexdigest()[:8] + module_name = f'rfm_{module_name}_{module_hash}' + if module_name in sys.modules: return sys.modules[module_name] @@ -72,7 +74,7 @@ def _do_import_module_from_file(filename, module_name=None): return module -def import_module_from_file(filename, force=False): +def import_module_from_file(filename, force=False, unique_name=False): '''Import module from file. :arg filename: The path to the filename of a Python module. @@ -91,7 +93,7 @@ def import_module_from_file(filename, force=False): if rel_filename.startswith('..'): # We cannot use the standard Python import mechanism here, because the # module to import is outside the top-level package - return _do_import_module_from_file(filename, module_name) + return _do_import_module_from_file(filename, module_name, unique_name) # Extract module name if `filename` is under `site-packages/` or the # Debian specific `dist-packages/` diff --git a/unittests/test_utility.py b/unittests/test_utility.py index b0e997b8bc..cef06b30b3 100644 --- a/unittests/test_utility.py +++ b/unittests/test_utility.py @@ -18,6 +18,8 @@ import reframe.utility.sanity as sn import unittests.utility as test_util +from hashlib import sha256 + from reframe.core.exceptions import (ConfigError, SpawnedProcessError, SpawnedProcessTimeout) @@ -488,6 +490,16 @@ def test_import_from_file_load_unknown_path(): assert '/foo' == e.path +def test_import_from_file_load_unknown_path_unique_name(): + try: + util.import_module_from_file('/foo', unique_name=True) + pytest.fail() + except ImportError as e: + module_hash = sha256('/foo'.encode('utf-8')).hexdigest()[:8] + assert f'rfm_foo_{module_hash}' == e.name + assert '/foo' == e.path + + def test_import_from_file_load_directory_relative(): with osext.change_dir('reframe'): module = util.import_module_from_file('../reframe') From b4870953c58671e825d2e045d22114d5675c988c Mon Sep 17 00:00:00 2001 From: Rafael Sarmiento Date: Fri, 21 Jan 2022 13:15:07 +0100 Subject: [PATCH 3/4] fix comments --- reframe/utility/__init__.py | 13 ++++++------- unittests/test_loader.py | 30 ++++++++++++++++++++++++++++++ unittests/test_utility.py | 37 ++++++++----------------------------- 3 files changed, 44 insertions(+), 36 deletions(-) diff --git a/reframe/utility/__init__.py b/reframe/utility/__init__.py index e64ba1cd46..279f7115e9 100644 --- a/reframe/utility/__init__.py +++ b/reframe/utility/__init__.py @@ -54,12 +54,8 @@ def _get_module_name(filename): return barename.replace(os.sep, '.') -def _do_import_module_from_file(filename, module_name=None, unique_name=False): +def _do_import_module_from_file(filename, module_name=None): module_name = module_name or _get_module_name(filename) - if unique_name: - module_hash = sha256(filename.encode('utf-8')).hexdigest()[:8] - module_name = f'rfm_{module_name}_{module_hash}' - if module_name in sys.modules: return sys.modules[module_name] @@ -74,7 +70,7 @@ def _do_import_module_from_file(filename, module_name=None, unique_name=False): return module -def import_module_from_file(filename, force=False, unique_name=False): +def import_module_from_file(filename, force=False): '''Import module from file. :arg filename: The path to the filename of a Python module. @@ -93,7 +89,10 @@ def import_module_from_file(filename, force=False, unique_name=False): if rel_filename.startswith('..'): # We cannot use the standard Python import mechanism here, because the # module to import is outside the top-level package - return _do_import_module_from_file(filename, module_name, unique_name) + module_hash = sha256(filename.encode('utf-8')).hexdigest()[:8] + module_name = f'rfm_{module_name}_{module_hash}' + + return _do_import_module_from_file(filename, module_name) # Extract module name if `filename` is under `site-packages/` or the # Debian specific `dist-packages/` diff --git a/unittests/test_loader.py b/unittests/test_loader.py index 286548ed5f..afd0a6fe06 100644 --- a/unittests/test_loader.py +++ b/unittests/test_loader.py @@ -5,6 +5,7 @@ import os import pytest +import shutil import reframe as rfm from reframe.core.exceptions import ReframeSyntaxError @@ -24,6 +25,21 @@ def loader_with_path(): ) +@pytest.fixture +def loader_with_path_tmpdir(tmp_path): + test_dir_a = tmp_path / 'a' + test_dir_b = tmp_path / 'b' + os.mkdir(test_dir_a) + os.mkdir(test_dir_b) + test_a = 'unittests/resources/checks/emptycheck.py' + test_b = 'unittests/resources/checks/hellocheck.py' + shutil.copyfile(test_a, test_dir_a / 'test.py') + shutil.copyfile(test_b, test_dir_b / 'test.py') + return RegressionCheckLoader( + [test_dir_a.as_posix(), test_dir_b.as_posix()] + ) + + def test_load_file_relative(loader): checks = loader.load_from_file('unittests/resources/checks/emptycheck.py') assert 1 == len(checks) @@ -73,6 +89,20 @@ def test_load_fixtures(loader): assert 5 == len(tests) +def test_existing_module_name(loader, tmp_path): + print(type(tmp_path)) + test_file = tmp_path / 'os.py' + shutil.copyfile('unittests/resources/checks/emptycheck.py', test_file) + checks = loader.load_from_file(test_file) + assert 1 == len(checks) + assert checks[0].name == 'EmptyTest' + + +def test_same_filename_different_path(loader_with_path_tmpdir): + checks = loader_with_path_tmpdir.load_all() + assert 3 == len(checks) + + def test_special_test(): with pytest.raises(ReframeSyntaxError): @rfm.simple_test diff --git a/unittests/test_utility.py b/unittests/test_utility.py index cef06b30b3..f6c81c694d 100644 --- a/unittests/test_utility.py +++ b/unittests/test_utility.py @@ -18,8 +18,6 @@ import reframe.utility.sanity as sn import unittests.utility as test_util -from hashlib import sha256 - from reframe.core.exceptions import (ConfigError, SpawnedProcessError, SpawnedProcessTimeout) @@ -481,23 +479,15 @@ def test_import_from_file_load_abspath(): assert module is sys.modules.get('reframe') -def test_import_from_file_load_unknown_path(): - try: - util.import_module_from_file('/foo') - pytest.fail() - except ImportError as e: - assert 'foo' == e.name - assert '/foo' == e.path - +def test_import_from_file_existing_module_name(tmp_path): + test_file = tmp_path / 'os.py' + with open(test_file, 'w') as fp: + print('var = 1', file=fp) -def test_import_from_file_load_unknown_path_unique_name(): - try: - util.import_module_from_file('/foo', unique_name=True) - pytest.fail() - except ImportError as e: - module_hash = sha256('/foo'.encode('utf-8')).hexdigest()[:8] - assert f'rfm_foo_{module_hash}' == e.name - assert '/foo' == e.path + module = util.import_module_from_file(test_file) + assert module.var == 1 + assert not hasattr(module, 'abc') + assert hasattr(os, 'abc') def test_import_from_file_load_directory_relative(): @@ -522,17 +512,6 @@ def test_import_from_file_load_relative(): assert module is sys.modules.get('reframe.utility.osext') -def test_import_from_file_load_outside_pkg(): - module = util.import_module_from_file(os.path.__file__) - - # os imports the OS-specific path libraries under the name `path`. Our - # importer will import the actual file, thus the module name should be - # the real one. - assert (module is sys.modules.get('posixpath') or - module is sys.modules.get('ntpath') or - module is sys.modules.get('macpath')) - - def test_import_from_file_load_twice(): filename = os.path.abspath('reframe') module1 = util.import_module_from_file(filename) From 2955791d7b4ca5c3d692404ecc25f98629b42278 Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Fri, 21 Jan 2022 23:09:21 +0100 Subject: [PATCH 4/4] Address PR comments --- reframe/utility/__init__.py | 8 +++++--- unittests/test_loader.py | 1 - unittests/test_utility.py | 4 ++-- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/reframe/utility/__init__.py b/reframe/utility/__init__.py index 279f7115e9..d0aba8b887 100644 --- a/reframe/utility/__init__.py +++ b/reframe/utility/__init__.py @@ -88,10 +88,12 @@ def import_module_from_file(filename, force=False): module_name = _get_module_name(rel_filename) if rel_filename.startswith('..'): # We cannot use the standard Python import mechanism here, because the - # module to import is outside the top-level package + # module to import is outside the top-level package. We also mangle + # the name that we assign to the module, in order to avoid clashes + # with other modules loaded with a standard `import` or with multiple + # test files with the same name that reside in different directories. module_hash = sha256(filename.encode('utf-8')).hexdigest()[:8] - module_name = f'rfm_{module_name}_{module_hash}' - + module_name = f'{module_name}@{module_hash}' return _do_import_module_from_file(filename, module_name) # Extract module name if `filename` is under `site-packages/` or the diff --git a/unittests/test_loader.py b/unittests/test_loader.py index afd0a6fe06..2100f875d3 100644 --- a/unittests/test_loader.py +++ b/unittests/test_loader.py @@ -90,7 +90,6 @@ def test_load_fixtures(loader): def test_existing_module_name(loader, tmp_path): - print(type(tmp_path)) test_file = tmp_path / 'os.py' shutil.copyfile('unittests/resources/checks/emptycheck.py', test_file) checks = loader.load_from_file(test_file) diff --git a/unittests/test_utility.py b/unittests/test_utility.py index f6c81c694d..210889422f 100644 --- a/unittests/test_utility.py +++ b/unittests/test_utility.py @@ -486,8 +486,8 @@ def test_import_from_file_existing_module_name(tmp_path): module = util.import_module_from_file(test_file) assert module.var == 1 - assert not hasattr(module, 'abc') - assert hasattr(os, 'abc') + assert not hasattr(module, 'path') + assert hasattr(os, 'path') def test_import_from_file_load_directory_relative():