From aa4619a3ad3fc1070dcd7eea6f8dbc5c92f2a60d Mon Sep 17 00:00:00 2001 From: Cleber Rosa Date: Mon, 10 Jul 2023 16:25:28 -0400 Subject: [PATCH 1/5] avocado/core/utils/loader.py: remove check needed by legacy loader The legacy loader system could pass actual class objects instead of their names. This is no longer the case with the resolver, so the class name will always be a string. Signed-off-by: Cleber Rosa --- avocado/core/utils/loader.py | 52 ++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/avocado/core/utils/loader.py b/avocado/core/utils/loader.py index d95bc6c0fd..0490901c8a 100644 --- a/avocado/core/utils/loader.py +++ b/avocado/core/utils/loader.py @@ -36,32 +36,32 @@ def load_test(test_factory): test_path = test_parameters.pop("modulePath") else: test_path = None - if isinstance(test_class, str): - module_name = os.path.basename(test_path).split(".")[0] - test_module_dir = os.path.abspath(os.path.dirname(test_path)) - # Tests with local dir imports need this - try: - sys.path.insert(0, test_module_dir) - test_module = importlib.import_module(module_name) - except: # pylint: disable=W0702 - # On load_module exception we fake the test class and pass - # the exc_info as parameter to be logged. - test_parameters["methodName"] = "test" - exception = stacktrace.prepare_exc_info(sys.exc_info()) - test_parameters["exception"] = exception - return TestError(**test_parameters) - finally: - if test_module_dir in sys.path: - sys.path.remove(test_module_dir) - for _, obj in inspect.getmembers(test_module): - if ( - inspect.isclass(obj) - and obj.__name__ == test_class - and inspect.getmodule(obj) == test_module - ): - if issubclass(obj, test.Test): - test_class = obj - break + + module_name = os.path.basename(test_path).split(".")[0] + test_module_dir = os.path.abspath(os.path.dirname(test_path)) + # Tests with local dir imports need this + try: + sys.path.insert(0, test_module_dir) + test_module = importlib.import_module(module_name) + except: # pylint: disable=W0702 + # On load_module exception we fake the test class and pass + # the exc_info as parameter to be logged. + test_parameters["methodName"] = "test" + exception = stacktrace.prepare_exc_info(sys.exc_info()) + test_parameters["exception"] = exception + return TestError(**test_parameters) + finally: + if test_module_dir in sys.path: + sys.path.remove(test_module_dir) + for _, obj in inspect.getmembers(test_module): + if ( + inspect.isclass(obj) + and obj.__name__ == test_class + and inspect.getmodule(obj) == test_module + ): + if issubclass(obj, test.Test): + test_class = obj + break test_instance = test_class(**test_parameters) return test_instance From 6ad3c77b26ef72b9ce5cb9ef7a166fb49903d7c3 Mon Sep 17 00:00:00 2001 From: Cleber Rosa Date: Mon, 10 Jul 2023 16:25:28 -0400 Subject: [PATCH 2/5] avocado/core/utils/loader.py: remove unnecessary TestError Before nrunner, all test conditions (including errors) were "communicated" by means of specialized class inheriting from avocado.core.test.Test. With nrunner, the conditions are communicated using messages, and that is true for errors. The "avocado-instrumented" runner already protects against exceptions while trying to load/import test modules, so this extra handling is completely unnecessary. Signed-off-by: Cleber Rosa --- avocado/core/utils/loader.py | 22 ---------------------- 1 file changed, 22 deletions(-) diff --git a/avocado/core/utils/loader.py b/avocado/core/utils/loader.py index 0490901c8a..44348a3e62 100644 --- a/avocado/core/utils/loader.py +++ b/avocado/core/utils/loader.py @@ -4,21 +4,6 @@ import sys from avocado.core import test -from avocado.utils import stacktrace - - -class TestError(test.Test): - """ - Generic test error. - """ - - def __init__(self, *args, **kwargs): - exception = kwargs.pop("exception") - test.Test.__init__(self, *args, **kwargs) - self.exception = exception - - def test(self): - self.error(self.exception) def load_test(test_factory): @@ -43,13 +28,6 @@ def load_test(test_factory): try: sys.path.insert(0, test_module_dir) test_module = importlib.import_module(module_name) - except: # pylint: disable=W0702 - # On load_module exception we fake the test class and pass - # the exc_info as parameter to be logged. - test_parameters["methodName"] = "test" - exception = stacktrace.prepare_exc_info(sys.exc_info()) - test_parameters["exception"] = exception - return TestError(**test_parameters) finally: if test_module_dir in sys.path: sys.path.remove(test_module_dir) From 31a37a7e773ebf770f6abe9702213a0cb7e839de Mon Sep 17 00:00:00 2001 From: Cleber Rosa Date: Mon, 10 Jul 2023 16:25:28 -0400 Subject: [PATCH 3/5] avocado/core/utils/loader.py: flag lack of modulePath The load_test() function simply won't work without the path for the Python module to be imported. Instead of failing cryptically when trying to do `os.path.basename(test_path)` with test_path being None, let's flag that situation and report an error early. Signed-off-by: Cleber Rosa --- avocado/core/utils/loader.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/avocado/core/utils/loader.py b/avocado/core/utils/loader.py index 44348a3e62..79959a55c8 100644 --- a/avocado/core/utils/loader.py +++ b/avocado/core/utils/loader.py @@ -17,11 +17,12 @@ def load_test(test_factory): test_class, test_parameters = test_factory if "run.results_dir" in test_parameters: test_parameters["base_logdir"] = test_parameters.pop("run.results_dir") - if "modulePath" in test_parameters: - test_path = test_parameters.pop("modulePath") - else: - test_path = None + if "modulePath" not in test_parameters: + raise RuntimeError( + 'Test factory parameters is missing the module\'s path ("modulePath")' + ) + test_path = test_parameters.pop("modulePath") module_name = os.path.basename(test_path).split(".")[0] test_module_dir = os.path.abspath(os.path.dirname(test_path)) # Tests with local dir imports need this From fc6edff7c899c6815eb82d36a64b737a4a04bce5 Mon Sep 17 00:00:00 2001 From: Cleber Rosa Date: Mon, 10 Jul 2023 16:25:28 -0400 Subject: [PATCH 4/5] avocado/core/utils/loader.py: raise clear error when failing to import If we fail to import the module we want, and find the class we want, let's raise a clear exception that will be given to users. Signed-off-by: Cleber Rosa --- avocado/core/utils/loader.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/avocado/core/utils/loader.py b/avocado/core/utils/loader.py index 79959a55c8..398b141960 100644 --- a/avocado/core/utils/loader.py +++ b/avocado/core/utils/loader.py @@ -37,10 +37,7 @@ def load_test(test_factory): inspect.isclass(obj) and obj.__name__ == test_class and inspect.getmodule(obj) == test_module + and issubclass(obj, test.Test) ): - if issubclass(obj, test.Test): - test_class = obj - break - test_instance = test_class(**test_parameters) - - return test_instance + return obj(**test_parameters) + raise ImportError(f'Failed to find/load class "{test_class}" in "{test_path}"') From f2007a6558b55c3bc6c29b6b0fe99d286296b185 Mon Sep 17 00:00:00 2001 From: Cleber Rosa Date: Mon, 10 Jul 2023 16:25:28 -0400 Subject: [PATCH 5/5] avocado/core/utils/loader.py: respect the path of the module file The `sys.path.insert()` strategy being used up to this point is actually ineffective when the same module name exists elsewhere. With this change, explicit path of the Python file that contains the test is used to describe what module to load. Fixes: https://github.com/avocado-framework/avocado/issues/5686 Signed-off-by: Cleber Rosa --- avocado/core/utils/loader.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/avocado/core/utils/loader.py b/avocado/core/utils/loader.py index 398b141960..965a7bdbfc 100644 --- a/avocado/core/utils/loader.py +++ b/avocado/core/utils/loader.py @@ -25,10 +25,13 @@ def load_test(test_factory): test_path = test_parameters.pop("modulePath") module_name = os.path.basename(test_path).split(".")[0] test_module_dir = os.path.abspath(os.path.dirname(test_path)) - # Tests with local dir imports need this + spec = importlib.util.spec_from_file_location(module_name, test_path) + test_module = importlib.util.module_from_spec(spec) + sys.modules[module_name] = test_module try: + # Tests with local dir imports need this sys.path.insert(0, test_module_dir) - test_module = importlib.import_module(module_name) + spec.loader.exec_module(test_module) finally: if test_module_dir in sys.path: sys.path.remove(test_module_dir)