From 2adf3854feccc6bee409b38b5d80375c774c8925 Mon Sep 17 00:00:00 2001 From: Zhenyu Yao <111329301+zhenyu-ms@users.noreply.github.com> Date: Tue, 16 Jul 2024 11:18:50 +0800 Subject: [PATCH] Fix/issues related to driver dependency (#1109) * fix two bugs - fix exception handling within pre_resource steps - fix driver flag setting when driver dependency presents * have post_run_checks warning written to log file * address review comments; add support of dependencies=None * really address review comments; bugfix --- doc/en/multitest.rst | 5 +- ...anged.pre_resources_exception_handling.rst | 1 + testplan/common/report/base.py | 3 +- testplan/runnable/base.py | 5 +- testplan/testing/base.py | 4 +- testplan/testing/environment/base.py | 11 ++-- testplan/testing/multitest/base.py | 8 +++ .../runnable/interactive/test_interactive.py | 51 +++++++++++++++++++ .../unit/testplan/common/report/test_base.py | 12 ++++- .../testplan/testing/environment/test_base.py | 17 +++++++ .../testing/multitest/test_multitest.py | 21 ++++++++ 11 files changed, 126 insertions(+), 12 deletions(-) create mode 100644 doc/newsfragments/3005_changed.pre_resources_exception_handling.rst 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 new file mode 100644 index 000000000..b6d29f7d4 --- /dev/null +++ b/doc/newsfragments/3005_changed.pre_resources_exception_handling.rst @@ -0,0 +1 @@ +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/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/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 diff --git a/testplan/testing/base.py b/testplan/testing/base.py index aa1d944ab..36281b868 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() @@ -430,11 +430,11 @@ def _build_environment(self) -> None: 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: diff --git a/testplan/testing/environment/base.py b/testplan/testing/environment/base.py index fa1cff576..5c787c69f 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 @@ -54,9 +55,9 @@ 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__["_orig_dependency"] = None # Optional[DriverDepGraph] + self.__dict__["_rt_dependency"] = None # Optional[DriverDepGraph] + self.__dict__["_pocketwatches"] = {} # Dict[str, DriverPocketwatch] def set_dependency(self, dependency: Optional[DriverDepGraph]): if dependency is None: @@ -72,7 +73,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..eeaffdc8b 100644 --- a/testplan/testing/multitest/base.py +++ b/testplan/testing/multitest/base.py @@ -591,6 +591,14 @@ def skip_step(self, step) -> bool: or self.resources.stop_exceptions or self._get_error_logs() ) + 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, 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/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/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 5a26a6e61..607165dca 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,23 @@ 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 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