Fixing anti-patterns (cherry picks from PR 1039)

CONTRIBUTING.md

@@ -19,9 +19,14 @@ short summary of the most important parts:
   such as `id_`.
 * Always use `self` for the first argument to instance methods.
 * Always use `cls` for the first argument to class methods.
-* Use one leading underscore only for non-public methods and instance variables,
-  such as `_data`.
+* One leading underscore like `_data` is for non-public methods and instance
+  variables. And it can be used by sub-classes. If it won't be used in
+  sub-classes, use like `__data`.
+* If there is a pair of `get_x` and `set_x` methods, they should instead be a
+  proper property, which is easy to do with the built-in `@property` decorator.
 * Constants should be `CAPITALIZED_SNAKE_CASE`.
+* When importing a function, try to avoid renaming it with `import as` because
+  it introduces cognitive overhead to track yet another name.
 
 When in doubt, adhere to existing conventions, or check the style guide.
 
@@ -252,6 +257,18 @@ Python world. If you make it through even some of these guides, you will be well
 on your way to being a “Pythonista” (a Python developer) writing “Pythonic”
 (canonically correct Python) code left and right.
 
+### Async IO
+
+With Python 3.4, the Async IO pattern found in languages such as C# and Go is
+available through the keywords `async` and `await`, along with the Python module
+`asyncio`. Please read [Async IO in Python: A Complete
+Walkthrough](https://realpython.com/async-io-python/) to understand at a high
+level how asynchronous programming works. As of Python 3.7, One major “gotcha”
+is that `asyncio.run(...)` should be used [exactly once in
+`main`](https://docs.python.org/3/library/asyncio-task.html), it starts the
+event loop. Everything else should be a coroutine or task which the event loop
+schedules.
+
 ## Future Sections
 
 Just a collection of reminders for the author to expand on later.

Makefile

@@ -1,18 +1,18 @@
 # This Makefile simply automates all our tasks. Its use is optional.
 
-all: setup run check
+all: setup run test check
 
 # Install Python packages
 setup:
 	@poetry install --no-ansi --remove-untracked
 
 # Run LISAv3
 run:
-	@poetry run python lisa/main.py --debug
+	@poetry run python -X dev lisa/main.py --debug
 
 # Run unit tests
 test:
-	@poetry run python -m unittest discover lisa
+	@poetry run python -X dev -m unittest discover -v lisa
 
 # Generate coverage report (slow, reruns LISAv3 and tests)
 coverage:

lisa/action.py

@@ -47,7 +47,7 @@ def __init__(self) -> None:
     @abstractmethod
     async def start(self) -> None:
         self.__is_started = True
-        self.set_status(ActionStatus.RUNNING)
+        self.status = ActionStatus.RUNNING
 
     @abstractmethod
     async def stop(self) -> None:
@@ -57,25 +57,28 @@ async def stop(self) -> None:
     async def close(self) -> None:
         self.validate_started()
 
-    def get_status(self) -> ActionStatus:
+    @property
+    def status(self) -> ActionStatus:
+        """The Action's current state, for example, 'UNINITIALIZED'."""
         return self.__status
 
-    def set_status(self, status: ActionStatus) -> None:
-        if self.__status != status:
+    @status.setter
+    def status(self, value: ActionStatus) -> None:
+        if self.__status != value:
             self.log.debug(
                 f"{self.name} status changed from {self.__status.name} "
-                f"to {status.name} with {self.__timer}"
+                f"to {value.name} with {self.__timer}"
             )
             self.__total += self.__timer.elapsed()
             message = ActionMessage(
                 elapsed=self.__timer.elapsed(),
                 sub_type=self.name,
-                status=status,
+                status=value,
                 total_elapsed=self.__total,
             )
             notifier.notify(message=message)
             self.__timer = create_timer()
-        self.__status = status
+            self.__status = value
 
     def validate_started(self) -> None:
         if not self.__is_started:

lisa/commands.py

@@ -1,11 +1,10 @@
-import asyncio
 import functools
 from argparse import Namespace
 from typing import Iterable, Optional, cast
 
 from lisa import notifier
-from lisa.lisarunner import LisaRunner
-from lisa.parameter_parser.runbook import load as load_runbook
+from lisa.parameter_parser.runbook import load_runbook
+from lisa.runner import Runner
 from lisa.testselector import select_testcases
 from lisa.testsuite import TestCaseRuntimeData
 from lisa.util import LisaException, constants
@@ -14,29 +13,28 @@
 _get_init_logger = functools.partial(get_logger, "init")
 
 
-def run(args: Namespace) -> int:
-    runbook = load_runbook(args)
+async def run(args: Namespace) -> int:
+    runbook = load_runbook(args.runbook, args.variables)
 
     if runbook.notifier:
         notifier.initialize(runbooks=runbook.notifier)
     try:
-        runner = LisaRunner(runbook)
-        awaitable = runner.start()
-        asyncio.run(awaitable)
+        runner = Runner(runbook)
+        await runner.start()
     finally:
         notifier.finalize()
 
     return runner.exit_code
 
 
 # check runbook
-def check(args: Namespace) -> int:
-    load_runbook(args)
+async def check(args: Namespace) -> int:
+    load_runbook(args.runbook, args.variables)
     return 0
 
 
-def list_start(args: Namespace) -> int:
-    runbook = load_runbook(args)
+async def list_start(args: Namespace) -> int:
+    runbook = load_runbook(args.runbook, args.variables)
     list_all = cast(Optional[bool], args.list_all)
     log = _get_init_logger("list")
     if args.type == constants.LIST_CASE:

lisa/main.py

@@ -1,3 +1,4 @@
+import asyncio
 import sys
 import traceback
 from datetime import datetime
@@ -23,7 +24,7 @@ def create_run_path(root_path: Path) -> Path:
     return run_path
 
 
-def main() -> int:
+async def main() -> int:
     total_timer = create_timer()
     log = get_logger()
     exit_code: int = 0
@@ -57,7 +58,7 @@ def main() -> int:
         log.debug(f"command line args: {sys.argv}")
         log.info(f"run local path: {runtime_root}")
 
-        exit_code = args.func(args)
+        exit_code = await args.func(args)
         assert isinstance(exit_code, int), f"actual: {type(exit_code)}"
     finally:
         log.info(f"completed in {total_timer}")
@@ -68,7 +69,7 @@ def main() -> int:
 if __name__ == "__main__":
     exit_code = 0
     try:
-        exit_code = main()
+        exit_code = asyncio.run(main())
     except Exception as exception:
         exit_code = -1
         log = get_logger()

lisa/parameter_parser/argparser.py

@@ -1,4 +1,5 @@
 from argparse import ArgumentParser, Namespace
+from pathlib import Path
 
 from lisa import commands
 from lisa.util import constants
@@ -8,10 +9,10 @@ def support_runbook(parser: ArgumentParser, required: bool = True) -> None:
     parser.add_argument(
         "--runbook",
         "-r",
+        type=Path,
         required=required,
-        dest="runbook",
-        help="runbook of this run",
-        default="examples/runbook/hello_world.yml",
+        help="Path to the runbook",
+        default=Path("examples/runbook/hello_world.yml").absolute(),
     )
 
 
@@ -21,7 +22,7 @@ def support_debug(parser: ArgumentParser) -> None:
         "-d",
         dest="debug",
         action="store_true",
-        help="set log level to debug",
+        help="Set log level to debug",
     )
 
 
@@ -31,38 +32,39 @@ def support_variable(parser: ArgumentParser) -> None:
         "-v",
         dest="variables",
         action="append",
-        help="define variable from command line. format is NAME:VALUE",
+        help="Define one or more variables with 'NAME:VALUE'",
     )
 
 
 def parse_args() -> Namespace:
-    # parse args run function.
+    """This wraps Python's 'ArgumentParser' to setup our CLI."""
     parser = ArgumentParser()
     support_debug(parser)
     support_runbook(parser, required=False)
     support_variable(parser)
 
+    # Default to ‘run’ when no subcommand is given.
+    parser.set_defaults(func=commands.run)
+
     subparsers = parser.add_subparsers(dest="cmd", required=False)
+
+    # Entry point for ‘run’.
     run_parser = subparsers.add_parser("run")
     run_parser.set_defaults(func=commands.run)
-    support_runbook(run_parser)
-    support_variable(run_parser)
 
+    # Entry point for ‘list-start’.
     list_parser = subparsers.add_parser(constants.LIST)
     list_parser.set_defaults(func=commands.list_start)
     list_parser.add_argument("--type", "-t", dest="type", choices=["case"])
     list_parser.add_argument("--all", "-a", dest="list_all", action="store_true")
-    support_runbook(list_parser)
-    support_variable(list_parser)
 
+    # Entry point for ‘check’.
     check_parser = subparsers.add_parser("check")
     check_parser.set_defaults(func=commands.check)
-    support_runbook(check_parser)
-    support_variable(check_parser)
-
-    parser.set_defaults(func=commands.run)
 
     for sub_parser in subparsers.choices.values():
+        support_runbook(sub_parser)
+        support_variable(sub_parser)
         support_debug(sub_parser)
 
     return parser.parse_args()

lisa/parameter_parser/runbook.py

@@ -1,7 +1,6 @@
-from argparse import Namespace
 from functools import partial
 from pathlib import Path, PurePath
-from typing import Any, Dict, Optional, cast
+from typing import Any, Dict, List, Optional, cast
 
 import yaml
 from marshmallow import Schema
@@ -58,13 +57,13 @@ def validate_data(data: Any) -> schema.Runbook:
     return runbook
 
 
-def load(args: Namespace) -> schema.Runbook:
+def load_runbook(path: Path, user_variables: Optional[List[str]]) -> schema.Runbook:
+    """Loads a runbook given a user-supplied path and set of variables."""
     # make sure extension in lisa is loaded
     base_module_path = Path(__file__).parent.parent
     import_module(base_module_path, logDetails=False)
 
     # merge all parameters
-    path = Path(args.runbook).absolute()
     data = _load_data(path)
     constants.RUNBOOK_PATH = path.parent
 
@@ -73,14 +72,14 @@ def load(args: Namespace) -> schema.Runbook:
         extends_runbook = schema.Extension.schema().load(  # type:ignore
             data[constants.EXTENSION]
         )
-        _load_extends(path.parent, extends_runbook)
+        _load_extends(constants.RUNBOOK_PATH, extends_runbook)
 
     # load arg variables
     variables: Dict[str, Any] = dict()
+    # TODO: This is all side-effect driven and needs to be fixed.
     load_from_runbook(data, variables)
     load_from_env(variables)
-    if hasattr(args, "variables"):
-        load_from_pairs(args.variables, variables)
+    load_from_pairs(user_variables, variables)
 
     # replace variables:
     data = replace_variables(data, variables)

lisa/lisarunner.py → lisa/runner.py

@@ -7,7 +7,6 @@
 from lisa.testselector import select_testcases
 from lisa.testsuite import (
     TestCaseRequirement,
-    TestCaseRuntimeData,
     TestResult,
     TestStatus,
     TestSuite,
@@ -16,24 +15,28 @@
 from lisa.util.logger import get_logger
 
 
-class LisaRunner(Action):
+class Runner(Action):
     def __init__(self, runbook: schema.Runbook) -> None:
         super().__init__()
         self.exit_code: int = 0
 
         self._runbook = runbook
         self._log = get_logger("runner")
 
+    # TODO: This entire function is one long string of side-effects.
+    # We need to reduce this function's complexity to remove the
+    # disabled warning, and not rely solely on side effects.
     async def start(self) -> None:  # noqa: C901
-        # TODO: Reduce this function's complexity and remove the disabled warning.
         await super().start()
-        self.set_status(ActionStatus.RUNNING)
+        self.status = ActionStatus.RUNNING
 
         # select test cases
         selected_test_cases = select_testcases(self._runbook.testcase)
 
         # create test results
-        selected_test_results = self._create_test_results(selected_test_cases)
+        selected_test_results = [
+            TestResult(runtime_data=case) for case in selected_test_cases
+        ]
 
         # load predefined environments
         candidate_environments = load_environments(self._runbook.environment)
@@ -169,12 +172,13 @@ async def start(self) -> None:  # noqa: C901
                 continue
             self._log.info(f"    {key.name:<9}: {count}")
 
-        self.set_status(ActionStatus.SUCCESS)
+        self.status = ActionStatus.SUCCESS
 
         # pass failed count to exit code
         self.exit_code = result_count_dict.get(TestStatus.FAILED, 0)
 
         # for UT testability
+        self._latest_platform = platform
         self._latest_test_results = selected_test_results
 
     async def stop(self) -> None:
@@ -198,14 +202,6 @@ async def _run_suite(
             result.environment = environment
         await test_suite.start()
 
-    def _create_test_results(
-        self, cases: List[TestCaseRuntimeData]
-    ) -> List[TestResult]:
-        test_results: List[TestResult] = []
-        for x in cases:
-            test_results.append(TestResult(runtime_data=x))
-        return test_results
-
     def _merge_test_requirements(
         self,
         test_results: List[TestResult],