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

@@ -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 <multitest_drivers>` for more
-  information.
+  drivers to start concurrently. Using ``None`` here to specify the legacy
+  scheduling with ``async_start`` flags being set. Click :ref:`here <multitest_drivers>`
+  for more information.
 
 * **Runtime Information**: The environment always contains a member called
   ``runtime_info`` which contains information about the current state of the

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``.

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

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

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:

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."

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,

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"

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):

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))

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