From e0ce088f57ecaa218d39fd2fc24b591f3297e482 Mon Sep 17 00:00:00 2001 From: zhenyu-ms <111329301+zhenyu-ms@users.noreply.github.com> Date: Tue, 2 Jul 2024 17:13:48 +0800 Subject: [PATCH 1/4] fix two bugs - fix exception handling within pre_resource steps - fix driver flag setting when driver dependency presents --- ...anged.pre_resources_exception_handling.rst | 1 + testplan/testing/base.py | 46 ++++++++++------- testplan/testing/environment/base.py | 11 ++-- testplan/testing/multitest/base.py | 6 ++- .../runnable/interactive/test_interactive.py | 51 +++++++++++++++++++ .../testing/multitest/test_multitest.py | 24 +++++++++ 6 files changed, 117 insertions(+), 22 deletions(-) create mode 100644 doc/newsfragments/3005_changed.pre_resources_exception_handling.rst diff --git a/doc/newsfragments/3005_changed.pre_resources_exception_handling.rst b/doc/newsfragments/3005_changed.pre_resources_exception_handling.rst new file mode 100644 index 000000000..4d009fd15 --- /dev/null +++ b/doc/newsfragments/3005_changed.pre_resources_exception_handling.rst @@ -0,0 +1 @@ +Fix two runtime issues related to driver dependency setting. \ No newline at end of file diff --git a/testplan/testing/base.py b/testplan/testing/base.py index aa1d944ab..4352d5db6 100644 --- a/testplan/testing/base.py +++ b/testplan/testing/base.py @@ -416,33 +416,45 @@ def propagate_tag_indices(self) -> None: self.report.propagate_tag_indices() def _init_context(self) -> None: - if callable(self.cfg.initial_context): - self.resources._initial_context = self.cfg.initial_context() - else: - self.resources._initial_context = self.cfg.initial_context + try: + if callable(self.cfg.initial_context): + self.resources._initial_context = self.cfg.initial_context() + else: + self.resources._initial_context = self.cfg.initial_context + except Exception as e: + self.resources.self_exception = e + raise def _build_environment(self) -> None: # build environment only once in interactive mode if self._env_built: return - if callable(self.cfg.environment): - drivers = self.cfg.environment() - else: - drivers = self.cfg.environment + try: + if callable(self.cfg.environment): + drivers = self.cfg.environment() + else: + drivers = self.cfg.environment + for driver in drivers: + driver.parent = self + driver.cfg.parent = self.cfg + self.resources.add(driver) + except Exception as e: + self.resources.self_exception = e + raise - for driver in drivers: - driver.parent = self - driver.cfg.parent = self.cfg - self.resources.add(driver) self._env_built = True def _set_dependencies(self) -> None: - if callable(self.cfg.dependencies): - deps = parse_dependency(self.cfg.dependencies()) - else: - deps = self.cfg.dependencies - self.resources.set_dependency(deps) + try: + if callable(self.cfg.dependencies): + deps = parse_dependency(self.cfg.dependencies()) + else: + deps = self.cfg.dependencies + self.resources.set_dependency(deps) + except Exception as e: + self.resources.self_exception = e + raise def _start_resource(self) -> None: if len(self.resources) == 0: diff --git a/testplan/testing/environment/base.py b/testplan/testing/environment/base.py index fa1cff576..76dd2f36c 100644 --- a/testplan/testing/environment/base.py +++ b/testplan/testing/environment/base.py @@ -54,9 +54,12 @@ class TestEnvironment(Environment): def __init__(self, parent: Optional["Test"] = None): super().__init__(parent) - self.__dict__["_orig_dependency"]: Optional[DriverDepGraph] = None - self.__dict__["_rt_dependency"]: Optional[DriverDepGraph] = None - self.__dict__["_pocketwatches"]: Dict[str, DriverPocketwatch] = dict() + self.__dict__["self_exception"] = None # Optional[Exception] + self.__dict__["_orig_dependency"] = None # Optional[DriverDepGraph] + self.__dict__["_rt_dependency"] = None # Optional[DriverDepGraph] + self.__dict__[ + "_pocketwatches" + ] = dict() # Dict[str, DriverPocketwatch] def set_dependency(self, dependency: Optional[DriverDepGraph]): if dependency is None: @@ -72,7 +75,7 @@ def set_dependency(self, dependency: Optional[DriverDepGraph]): "while not being declared in `environment` parameter." ) for d in self._resources.values(): - if d.async_start != UNSET: + if d.cfg.async_start != UNSET: raise ValueError( f"`async_start` parameter of driver {d} should not " "be set if driver dependency is specified." diff --git a/testplan/testing/multitest/base.py b/testplan/testing/multitest/base.py index 05c041f7f..e8ed7f586 100644 --- a/testplan/testing/multitest/base.py +++ b/testplan/testing/multitest/base.py @@ -587,10 +587,14 @@ def skip_step(self, step) -> bool: """Check if a step should be skipped.""" if step == self._run_error_handler: return not ( - self.resources.start_exceptions + self.resources.self_exception is not None + or self.resources.start_exceptions or self.resources.stop_exceptions or self._get_error_logs() ) + elif self.resources.self_exception is not None: + # self of status ERROR + return True elif step in ( self._start_resource, self._stop_resource, diff --git a/tests/functional/testplan/runnable/interactive/test_interactive.py b/tests/functional/testplan/runnable/interactive/test_interactive.py index 94ec90ce1..41d1cc51a 100644 --- a/tests/functional/testplan/runnable/interactive/test_interactive.py +++ b/tests/functional/testplan/runnable/interactive/test_interactive.py @@ -430,3 +430,54 @@ def test_abort_handler(): timeout=5, raise_on_timeout=True, ) + + +def test_restart_multitest_w_dependencies(): + s = TCPServer(name="server") + c = TCPClient( + name="client", + host=context("server", "{{host}}"), + port=context("server", "{{port}}"), + ) + mt = MultiTest( + name="Test", + suites=[TCPSuite(0)], + environment=[s, c], + dependencies={s: c}, + after_start=lambda env: env.server.accept_connection(), + ) + + with InteractivePlan( + name="InteractivePlan", + interactive_port=0, + interactive_block=False, + parse_cmdline=False, + logger_level=USER_INFO, + ) as plan: + plan.add(mt) + plan.run() + wait_for_interactive_start(plan) + + plan.interactive.start_test_resources("Test") + plan.interactive.run_test("Test") + assert plan.interactive.test( + "Test" + ).run_result(), "no exceptions in steps" + assert plan.interactive.report[ + "Test" + ].unknown, "env stop remain unknown" + plan.interactive.stop_test_resources("Test") + assert plan.interactive.report["Test"].passed, "env stop succeeded" + + plan.interactive.reset_all_tests() + + plan.interactive.start_test_resources("Test") + plan.interactive.run_test("Test") + assert plan.interactive.test( + "Test" + ).run_result(), "no exceptions in steps" + assert plan.interactive.report[ + "Test" + ].unknown, "env stop remain unknown" + plan.interactive.stop_test_resources("Test") + assert plan.interactive.report["Test"].passed, "env stop succeeded" diff --git a/tests/unit/testplan/testing/multitest/test_multitest.py b/tests/unit/testplan/testing/multitest/test_multitest.py index 5a26a6e61..05733603c 100644 --- a/tests/unit/testplan/testing/multitest/test_multitest.py +++ b/tests/unit/testplan/testing/multitest/test_multitest.py @@ -11,6 +11,7 @@ from testplan.common.utils.thread import Barrier from testplan.testing import common, filtering, multitest, ordering from testplan.testing.multitest import base +from testplan.testing.multitest.driver import Driver MTEST_DEFAULT_PARAMS = { "test_filter": filtering.Filter(), @@ -597,3 +598,26 @@ def test_skip_strategy(skip_strategy, case_count): ) ret = mt.run() assert len(ret.report.entries[0]) == case_count + + +def test_skip_steps(): + s = Driver(name="server") + c = Driver( + name="client", + ) + mt = multitest.MultiTest( + name="Test", + suites=[Suite()], + environment=lambda: [s, c], + dependencies=lambda: {s: c, c: s}, + ) + + mt.run() + assert mt.resources.self_exception is not None + assert all( + map( + lambda x: x in mt.result.step_results, + ["_init_context", "_build_environment", "_set_dependencies"], + ) + ) + assert "_start_resource" not in mt.result.step_results From 9fb6112f9ab91ea8e9c1a2617d827c4ea47d647b Mon Sep 17 00:00:00 2001 From: zhenyu-ms <111329301+zhenyu-ms@users.noreply.github.com> Date: Wed, 3 Jul 2024 20:24:15 +0800 Subject: [PATCH 2/4] have post_run_checks warning written to log file --- testplan/runnable/base.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/testplan/runnable/base.py b/testplan/runnable/base.py index ac5557f6b..efee499a8 100644 --- a/testplan/runnable/base.py +++ b/testplan/runnable/base.py @@ -1078,7 +1078,6 @@ def add_post_resource_steps(self): self._add_step(self._invoke_exporters) self._add_step(self._post_exporters) self._add_step(self._stop_remote_services) - self._add_step(self._close_file_logger) super(TestRunner, self).add_post_resource_steps() self._add_step(self._stop_resource_monitor) @@ -1126,6 +1125,10 @@ def _wait_ongoing(self): break time.sleep(self.cfg.active_loop_sleep) + def _post_run_checks(self, start_threads, start_procs): + super()._post_run_checks(start_threads, start_procs) + self._close_file_logger() + def _create_result(self): """Fetch task result from executors and create a full test result.""" step_result = True From 6a97b35027bd79d607d4c91244e69f1a13121f6b Mon Sep 17 00:00:00 2001 From: zhenyu-ms <111329301+zhenyu-ms@users.noreply.github.com> Date: Wed, 10 Jul 2024 17:48:37 +0800 Subject: [PATCH 3/4] address review comments; add support of dependencies=None --- doc/en/multitest.rst | 5 +++-- ...changed.pre_resources_exception_handling.rst | 2 +- testplan/testing/base.py | 17 ++++------------- testplan/testing/environment/base.py | 15 +++++++++++---- .../testplan/testing/environment/test_base.py | 17 +++++++++++++++++ .../testing/multitest/test_multitest.py | 4 +--- 6 files changed, 37 insertions(+), 23 deletions(-) diff --git a/doc/en/multitest.rst b/doc/en/multitest.rst index 9ce6dee20..58106108e 100644 --- a/doc/en/multitest.rst +++ b/doc/en/multitest.rst @@ -70,8 +70,9 @@ A MultiTest instance can be constructed from the following parameters: starting concurrently based on the dependencies. Drivers included in ``environment`` while not presented in ``dependencies`` will be started concurrently at the very beginning. Using empty dict here to instruct all - drivers to start concurrently. Click :ref:`here ` for more - information. + drivers to start concurrently. Using ``None`` here to specify the legacy + scheduling with ``async_start`` flags being set. Click :ref:`here ` + for more information. * **Runtime Information**: The environment always contains a member called ``runtime_info`` which contains information about the current state of the diff --git a/doc/newsfragments/3005_changed.pre_resources_exception_handling.rst b/doc/newsfragments/3005_changed.pre_resources_exception_handling.rst index 4d009fd15..b6d29f7d4 100644 --- a/doc/newsfragments/3005_changed.pre_resources_exception_handling.rst +++ b/doc/newsfragments/3005_changed.pre_resources_exception_handling.rst @@ -1 +1 @@ -Fix two runtime issues related to driver dependency setting. \ No newline at end of file +Fix some runtime issues when driver dependency is set. ``dependencies`` argument of ``MultiTest`` now supports ``None``. \ No newline at end of file diff --git a/testplan/testing/base.py b/testplan/testing/base.py index 4352d5db6..8642d98d0 100644 --- a/testplan/testing/base.py +++ b/testplan/testing/base.py @@ -114,7 +114,7 @@ def get_options(cls): [Or(Resource, RemoteDriver)], validate_func() ), ConfigOption("dependencies", default=None): Or( - Use(parse_dependency), validate_func() + None, Use(parse_dependency), validate_func() ), ConfigOption("initial_context", default={}): Or( dict, validate_func() @@ -416,21 +416,18 @@ def propagate_tag_indices(self) -> None: self.report.propagate_tag_indices() def _init_context(self) -> None: - try: + with self.resources.set_self_exception(): if callable(self.cfg.initial_context): self.resources._initial_context = self.cfg.initial_context() else: self.resources._initial_context = self.cfg.initial_context - except Exception as e: - self.resources.self_exception = e - raise def _build_environment(self) -> None: # build environment only once in interactive mode if self._env_built: return - try: + with self.resources.set_self_exception(): if callable(self.cfg.environment): drivers = self.cfg.environment() else: @@ -439,22 +436,16 @@ def _build_environment(self) -> None: driver.parent = self driver.cfg.parent = self.cfg self.resources.add(driver) - except Exception as e: - self.resources.self_exception = e - raise self._env_built = True def _set_dependencies(self) -> None: - try: + with self.resources.set_self_exception(): if callable(self.cfg.dependencies): deps = parse_dependency(self.cfg.dependencies()) else: deps = self.cfg.dependencies self.resources.set_dependency(deps) - except Exception as e: - self.resources.self_exception = e - raise def _start_resource(self) -> None: if len(self.resources) == 0: diff --git a/testplan/testing/environment/base.py b/testplan/testing/environment/base.py index 76dd2f36c..a53487dcc 100644 --- a/testplan/testing/environment/base.py +++ b/testplan/testing/environment/base.py @@ -1,7 +1,8 @@ import copy import time +from contextlib import contextmanager from dataclasses import dataclass -from typing import TYPE_CHECKING, Dict, Optional +from typing import TYPE_CHECKING, Optional from testplan.common.config import UNSET from testplan.common.entity.base import Environment @@ -57,9 +58,15 @@ def __init__(self, parent: Optional["Test"] = None): self.__dict__["self_exception"] = None # Optional[Exception] self.__dict__["_orig_dependency"] = None # Optional[DriverDepGraph] self.__dict__["_rt_dependency"] = None # Optional[DriverDepGraph] - self.__dict__[ - "_pocketwatches" - ] = dict() # Dict[str, DriverPocketwatch] + self.__dict__["_pocketwatches"] = {} # Dict[str, DriverPocketwatch] + + @contextmanager + def set_self_exception(self): + try: + yield + except Exception as e: + self.self_exception = e + raise def set_dependency(self, dependency: Optional[DriverDepGraph]): if dependency is None: diff --git a/tests/unit/testplan/testing/environment/test_base.py b/tests/unit/testplan/testing/environment/test_base.py index 55a6b3685..61b32ec82 100644 --- a/tests/unit/testplan/testing/environment/test_base.py +++ b/tests/unit/testplan/testing/environment/test_base.py @@ -37,6 +37,23 @@ def test_legacy_driver_scheduling(mocker): env.stop() +def test_set_dependency_none(mocker): + m = mocker.Mock() + env = TestEnvironment() + env.add(MockDriver("a", m, async_start=True)) + env.add(MockDriver("b", m)) + env.set_dependency(None) + env.start() + m.assert_has_calls( + [ + mocker.call.pre("a"), + mocker.call.pre("b"), + mocker.call.post("b"), + mocker.call.post("a"), + ] + ) + + def test_bad_config_exception(): env = TestEnvironment() env.add(MockDriver("a", async_start=True)) diff --git a/tests/unit/testplan/testing/multitest/test_multitest.py b/tests/unit/testplan/testing/multitest/test_multitest.py index 05733603c..853d6aa12 100644 --- a/tests/unit/testplan/testing/multitest/test_multitest.py +++ b/tests/unit/testplan/testing/multitest/test_multitest.py @@ -602,9 +602,7 @@ def test_skip_strategy(skip_strategy, case_count): def test_skip_steps(): s = Driver(name="server") - c = Driver( - name="client", - ) + c = Driver(name="client") mt = multitest.MultiTest( name="Test", suites=[Suite()], From 1356e61c584d0b8b07a9e802ddef2d560ba2cbdd Mon Sep 17 00:00:00 2001 From: zhenyu-ms <111329301+zhenyu-ms@users.noreply.github.com> Date: Fri, 12 Jul 2024 18:12:02 +0800 Subject: [PATCH 4/4] really address review comments; bugfix --- testplan/common/report/base.py | 3 +- testplan/testing/base.py | 37 +++++++++---------- testplan/testing/environment/base.py | 9 ----- testplan/testing/multitest/base.py | 12 ++++-- .../unit/testplan/common/report/test_base.py | 12 +++++- .../testing/multitest/test_multitest.py | 1 - 6 files changed, 38 insertions(+), 36 deletions(-) diff --git a/testplan/common/report/base.py b/testplan/common/report/base.py index b570b7415..24bf2d9e1 100644 --- a/testplan/common/report/base.py +++ b/testplan/common/report/base.py @@ -70,6 +70,7 @@ def __exit__(self, exc_type, exc_value, tb): str(exc_value), ) self.report.status_override = Status.SKIPPED + return True elif issubclass(exc_type, self.exception_classes): # Custom exception message with extra args exc_msg = "".join( @@ -79,7 +80,7 @@ def __exit__(self, exc_type, exc_value, tb): if self.fail: self.report.status_override = Status.ERROR - return True + return True @total_ordering diff --git a/testplan/testing/base.py b/testplan/testing/base.py index 8642d98d0..36281b868 100644 --- a/testplan/testing/base.py +++ b/testplan/testing/base.py @@ -416,36 +416,33 @@ def propagate_tag_indices(self) -> None: self.report.propagate_tag_indices() def _init_context(self) -> None: - with self.resources.set_self_exception(): - if callable(self.cfg.initial_context): - self.resources._initial_context = self.cfg.initial_context() - else: - self.resources._initial_context = self.cfg.initial_context + if callable(self.cfg.initial_context): + self.resources._initial_context = self.cfg.initial_context() + else: + self.resources._initial_context = self.cfg.initial_context def _build_environment(self) -> None: # build environment only once in interactive mode if self._env_built: return - with self.resources.set_self_exception(): - if callable(self.cfg.environment): - drivers = self.cfg.environment() - else: - drivers = self.cfg.environment - for driver in drivers: - driver.parent = self - driver.cfg.parent = self.cfg - self.resources.add(driver) + if callable(self.cfg.environment): + drivers = self.cfg.environment() + else: + drivers = self.cfg.environment + for driver in drivers: + driver.parent = self + driver.cfg.parent = self.cfg + self.resources.add(driver) self._env_built = True def _set_dependencies(self) -> None: - with self.resources.set_self_exception(): - if callable(self.cfg.dependencies): - deps = parse_dependency(self.cfg.dependencies()) - else: - deps = self.cfg.dependencies - self.resources.set_dependency(deps) + if callable(self.cfg.dependencies): + deps = parse_dependency(self.cfg.dependencies()) + else: + deps = self.cfg.dependencies + self.resources.set_dependency(deps) def _start_resource(self) -> None: if len(self.resources) == 0: diff --git a/testplan/testing/environment/base.py b/testplan/testing/environment/base.py index a53487dcc..5c787c69f 100644 --- a/testplan/testing/environment/base.py +++ b/testplan/testing/environment/base.py @@ -55,19 +55,10 @@ class TestEnvironment(Environment): def __init__(self, parent: Optional["Test"] = None): super().__init__(parent) - self.__dict__["self_exception"] = None # Optional[Exception] self.__dict__["_orig_dependency"] = None # Optional[DriverDepGraph] self.__dict__["_rt_dependency"] = None # Optional[DriverDepGraph] self.__dict__["_pocketwatches"] = {} # Dict[str, DriverPocketwatch] - @contextmanager - def set_self_exception(self): - try: - yield - except Exception as e: - self.self_exception = e - raise - def set_dependency(self, dependency: Optional[DriverDepGraph]): if dependency is None: return diff --git a/testplan/testing/multitest/base.py b/testplan/testing/multitest/base.py index e8ed7f586..eeaffdc8b 100644 --- a/testplan/testing/multitest/base.py +++ b/testplan/testing/multitest/base.py @@ -587,13 +587,17 @@ def skip_step(self, step) -> bool: """Check if a step should be skipped.""" if step == self._run_error_handler: return not ( - self.resources.self_exception is not None - or self.resources.start_exceptions + self.resources.start_exceptions or self.resources.stop_exceptions or self._get_error_logs() ) - elif self.resources.self_exception is not None: - # self of status ERROR + elif "_start_resource" not in self.result.step_results and any( + map( + lambda x: isinstance(x, Exception), + self.result.step_results.values(), + ) + ): + # exc before _start_resource return True elif step in ( self._start_resource, diff --git a/tests/unit/testplan/common/report/test_base.py b/tests/unit/testplan/common/report/test_base.py index c13c7bf1d..cda4f2ea9 100644 --- a/tests/unit/testplan/common/report/test_base.py +++ b/tests/unit/testplan/common/report/test_base.py @@ -37,7 +37,7 @@ def test_exception_logger_suppression(): @disable_log_propagation(LOGGER) def test_exception_logger_reraise(): """ - ExceptionLoggerBase should raise the exception without logging + ExceptionLogger.* should raise the exception without logging if it doesn't match `exception_classes`. """ rep = DummyReport() @@ -50,6 +50,16 @@ def test_exception_logger_reraise(): with rep.logged_exceptions(IndexError): raise KeyError("bar") # raised + rep = DummyReportGroup() + + with pytest.raises(KeyError): + + with rep.logged_exceptions(IndexError): + raise IndexError("foo") # suppressed + + with rep.logged_exceptions(IndexError): + raise KeyError("bar") # raised + class TestReport: def test_equality(self): diff --git a/tests/unit/testplan/testing/multitest/test_multitest.py b/tests/unit/testplan/testing/multitest/test_multitest.py index 853d6aa12..607165dca 100644 --- a/tests/unit/testplan/testing/multitest/test_multitest.py +++ b/tests/unit/testplan/testing/multitest/test_multitest.py @@ -611,7 +611,6 @@ def test_skip_steps(): ) mt.run() - assert mt.resources.self_exception is not None assert all( map( lambda x: x in mt.result.step_results,