For the whole series:
Reviewed-by: Jeremy Spewock <jspew...@iol.unh.edu>
On Fri, Mar 1, 2024 at 5:55 AM Juraj Linkeš <juraj.lin...@pantheon.tech> wrote:
>
> The two places where we specify which test suite and test cases to run
> are complimentary and not that intuitive to use. A unified way provides
> a better user experience.
>
> The syntax in test run configuration file has not changed, but the
> environment variable and the command line arguments was changed to match
> the config file syntax. This required changes in the settings module
> which greatly simplified the parsing of the environment variables while
> retaining the same functionality.
>
> Signed-off-by: Juraj Linkeš <juraj.lin...@pantheon.tech>
> ---
> doc/guides/tools/dts.rst | 14 ++-
> dts/framework/config/__init__.py | 12 +-
> dts/framework/runner.py | 18 +--
> dts/framework/settings.py | 187 ++++++++++++++-----------------
> dts/framework/test_suite.py | 2 +-
> 5 files changed, 114 insertions(+), 119 deletions(-)
>
> diff --git a/doc/guides/tools/dts.rst b/doc/guides/tools/dts.rst
> index f686ca487c..d1c3c2af7a 100644
> --- a/doc/guides/tools/dts.rst
> +++ b/doc/guides/tools/dts.rst
> @@ -215,28 +215,30 @@ DTS is run with ``main.py`` located in the ``dts``
> directory after entering Poet
> .. code-block:: console
>
> (dts-py3.10) $ ./main.py --help
> - usage: main.py [-h] [--config-file CONFIG_FILE] [--output-dir OUTPUT_DIR]
> [-t TIMEOUT] [-v] [-s] [--tarball TARBALL] [--compile-timeout
> COMPILE_TIMEOUT] [--test-cases TEST_CASES] [--re-run RE_RUN]
> + usage: main.py [-h] [--config-file CONFIG_FILE] [--output-dir OUTPUT_DIR]
> [-t TIMEOUT] [-v] [-s] [--tarball TARBALL] [--compile-timeout
> COMPILE_TIMEOUT] [--test-suite TEST_SUITE [TEST_CASES ...]] [--re-run RE_RUN]
>
> Run DPDK test suites. All options may be specified with the environment
> variables provided in brackets. Command line arguments have higher priority.
>
> options:
> -h, --help show this help message and exit
> --config-file CONFIG_FILE
> - [DTS_CFG_FILE] configuration file that describes
> the test cases, SUTs and targets. (default: ./conf.yaml)
> + [DTS_CFG_FILE] The configuration file that
> describes the test cases, SUTs and targets. (default: ./conf.yaml)
> --output-dir OUTPUT_DIR, --output OUTPUT_DIR
> [DTS_OUTPUT_DIR] Output directory where DTS logs
> and results are saved. (default: output)
> -t TIMEOUT, --timeout TIMEOUT
> [DTS_TIMEOUT] The default timeout for all DTS
> operations except for compiling DPDK. (default: 15)
> -v, --verbose [DTS_VERBOSE] Specify to enable verbose output,
> logging all messages to the console. (default: False)
> - -s, --skip-setup [DTS_SKIP_SETUP] Specify to skip all setup steps on
> SUT and TG nodes. (default: None)
> + -s, --skip-setup [DTS_SKIP_SETUP] Specify to skip all setup steps on
> SUT and TG nodes. (default: False)
> --tarball TARBALL, --snapshot TARBALL, --git-ref TARBALL
> [DTS_DPDK_TARBALL] Path to DPDK source code tarball
> or a git commit ID, tag ID or tree ID to test. To test local changes, first
> commit them, then use the commit ID with this option. (default: dpdk.tar.xz)
> --compile-timeout COMPILE_TIMEOUT
> [DTS_COMPILE_TIMEOUT] The timeout for compiling
> DPDK. (default: 1200)
> - --test-cases TEST_CASES
> - [DTS_TESTCASES] Comma-separated list of test cases
> to execute. Unknown test cases will be silently ignored. (default: )
> + --test-suite TEST_SUITE [TEST_CASES ...]
> + [DTS_TEST_SUITES] A list containing a test suite
> with test cases. The first parameter is the test suite name, and the rest are
> test case names, which are optional. May be specified multiple times. To
> specify multiple test suites in the environment
> + variable, join the lists with a comma. Examples:
> --test-suite suite case case --test-suite suite case ... |
> DTS_TEST_SUITES='suite case case, suite case, ...' | --test-suite suite
> --test-suite suite case ... | DTS_TEST_SUITES='suite, suite case, ...'
> + (default: [])
> --re-run RE_RUN, --re_run RE_RUN
> - [DTS_RERUN] Re-run each test case the specified
> number of times if a test failure occurs (default: 0)
> + [DTS_RERUN] Re-run each test case the specified
> number of times if a test failure occurs. (default: 0)
>
>
> The brackets contain the names of environment variables that set the same
> thing.
> diff --git a/dts/framework/config/__init__.py
> b/dts/framework/config/__init__.py
> index c6a93b3b89..4cb5c74059 100644
> --- a/dts/framework/config/__init__.py
> +++ b/dts/framework/config/__init__.py
> @@ -35,9 +35,9 @@
>
> import json
> import os.path
> -import pathlib
> from dataclasses import dataclass, fields
> from enum import auto, unique
> +from pathlib import Path
> from typing import Union
>
> import warlock # type: ignore[import]
> @@ -53,7 +53,6 @@
> TrafficGeneratorConfigDict,
> )
> from framework.exception import ConfigurationError
> -from framework.settings import SETTINGS
> from framework.utils import StrEnum
>
>
> @@ -571,7 +570,7 @@ def from_dict(d: ConfigurationDict) -> "Configuration":
> return Configuration(executions=executions)
>
>
> -def load_config() -> Configuration:
> +def load_config(config_file_path: Path) -> Configuration:
> """Load DTS test run configuration from a file.
>
> Load the YAML test run configuration file
> @@ -581,13 +580,16 @@ def load_config() -> Configuration:
> The YAML test run configuration file is specified in the
> :option:`--config-file` command line
> argument or the :envvar:`DTS_CFG_FILE` environment variable.
>
> + Args:
> + config_file_path: The path to the YAML test run configuration file.
> +
> Returns:
> The parsed test run configuration.
> """
> - with open(SETTINGS.config_file_path, "r") as f:
> + with open(config_file_path, "r") as f:
> config_data = yaml.safe_load(f)
>
> - schema_path = os.path.join(pathlib.Path(__file__).parent.resolve(),
> "conf_yaml_schema.json")
> + schema_path = os.path.join(Path(__file__).parent.resolve(),
> "conf_yaml_schema.json")
>
> with open(schema_path, "r") as f:
> schema = json.load(f)
> diff --git a/dts/framework/runner.py b/dts/framework/runner.py
> index dfee8ebd7c..db8e3ba96b 100644
> --- a/dts/framework/runner.py
> +++ b/dts/framework/runner.py
> @@ -24,7 +24,7 @@
> import sys
> from pathlib import Path
> from types import MethodType
> -from typing import Iterable
> +from typing import Iterable, Sequence
>
> from .config import (
> BuildTargetConfiguration,
> @@ -83,7 +83,7 @@ class DTSRunner:
>
> def __init__(self):
> """Initialize the instance with configuration, logger, result and
> string constants."""
> - self._configuration = load_config()
> + self._configuration = load_config(SETTINGS.config_file_path)
> self._logger = get_dts_logger()
> if not os.path.exists(SETTINGS.output_dir):
> os.makedirs(SETTINGS.output_dir)
> @@ -129,7 +129,7 @@ def run(self):
> #. Execution teardown
>
> The test cases are filtered according to the specification in the
> test run configuration and
> - the :option:`--test-cases` command line argument or
> + the :option:`--test-suite` command line argument or
> the :envvar:`DTS_TESTCASES` environment variable.
> """
> sut_nodes: dict[str, SutNode] = {}
> @@ -147,7 +147,9 @@ def run(self):
> )
> execution_result = self._result.add_execution(execution)
> # we don't want to modify the original config, so create a
> copy
> - execution_test_suites = list(execution.test_suites)
> + execution_test_suites = list(
> + SETTINGS.test_suites if SETTINGS.test_suites else
> execution.test_suites
> + )
> if not execution.skip_smoke_tests:
> execution_test_suites[:0] =
> [TestSuiteConfig.from_dict("smoke_tests")]
> try:
> @@ -226,7 +228,7 @@ def _get_test_suites_with_cases(
> test_suite_class =
> self._get_test_suite_class(test_suite_config.test_suite)
> test_cases = []
> func_test_cases, perf_test_cases = self._filter_test_cases(
> - test_suite_class, set(test_suite_config.test_cases +
> SETTINGS.test_cases)
> + test_suite_class, test_suite_config.test_cases
> )
> if func:
> test_cases.extend(func_test_cases)
> @@ -302,7 +304,7 @@ def is_test_suite(object) -> bool:
> )
>
> def _filter_test_cases(
> - self, test_suite_class: type[TestSuite], test_cases_to_run: set[str]
> + self, test_suite_class: type[TestSuite], test_cases_to_run:
> Sequence[str]
> ) -> tuple[list[MethodType], list[MethodType]]:
> """Filter `test_cases_to_run` from `test_suite_class`.
>
> @@ -331,7 +333,9 @@ def _filter_test_cases(
> (name, method) for name, method in name_method_tuples if
> name in test_cases_to_run
> ]
> if len(name_method_tuples) < len(test_cases_to_run):
> - missing_test_cases = test_cases_to_run - {name for name, _
> in name_method_tuples}
> + missing_test_cases = set(test_cases_to_run) - {
> + name for name, _ in name_method_tuples
> + }
> raise ConfigurationError(
> f"Test cases {missing_test_cases} not found among
> methods "
> f"of {test_suite_class.__name__}."
> diff --git a/dts/framework/settings.py b/dts/framework/settings.py
> index 2b8bfbe0ed..688e8679a7 100644
> --- a/dts/framework/settings.py
> +++ b/dts/framework/settings.py
> @@ -48,10 +48,11 @@
>
> The path to a DPDK tarball, git commit ID, tag ID or tree ID to test.
>
> -.. option:: --test-cases
> -.. envvar:: DTS_TESTCASES
> +.. option:: --test-suite
> +.. envvar:: DTS_TEST_SUITES
>
> - A comma-separated list of test cases to execute. Unknown test cases will
> be silently ignored.
> + A test suite with test cases which may be specified multiple times.
> + In the environment variable, the suites are joined with a comma.
>
> .. option:: --re-run, --re_run
> .. envvar:: DTS_RERUN
> @@ -71,83 +72,13 @@
>
> import argparse
> import os
> -from collections.abc import Callable, Iterable, Sequence
> from dataclasses import dataclass, field
> from pathlib import Path
> -from typing import Any, TypeVar
> +from typing import Any
>
> +from .config import TestSuiteConfig
> from .utils import DPDKGitTarball
>
> -_T = TypeVar("_T")
> -
> -
> -def _env_arg(env_var: str) -> Any:
> - """A helper method augmenting the argparse Action with environment
> variables.
> -
> - If the supplied environment variable is defined, then the default value
> - of the argument is modified. This satisfies the priority order of
> - command line argument > environment variable > default value.
> -
> - Arguments with no values (flags) should be defined using the const
> keyword argument
> - (True or False). When the argument is specified, it will be set to
> const, if not specified,
> - the default will be stored (possibly modified by the corresponding
> environment variable).
> -
> - Other arguments work the same as default argparse arguments, that is
> using
> - the default 'store' action.
> -
> - Returns:
> - The modified argparse.Action.
> - """
> -
> - class _EnvironmentArgument(argparse.Action):
> - def __init__(
> - self,
> - option_strings: Sequence[str],
> - dest: str,
> - nargs: str | int | None = None,
> - const: bool | None = None,
> - default: Any = None,
> - type: Callable[[str], _T | argparse.FileType | None] = None,
> - choices: Iterable[_T] | None = None,
> - required: bool = False,
> - help: str | None = None,
> - metavar: str | tuple[str, ...] | None = None,
> - ) -> None:
> - env_var_value = os.environ.get(env_var)
> - default = env_var_value or default
> - if const is not None:
> - nargs = 0
> - default = const if env_var_value else default
> - type = None
> - choices = None
> - metavar = None
> - super(_EnvironmentArgument, self).__init__(
> - option_strings,
> - dest,
> - nargs=nargs,
> - const=const,
> - default=default,
> - type=type,
> - choices=choices,
> - required=required,
> - help=help,
> - metavar=metavar,
> - )
> -
> - def __call__(
> - self,
> - parser: argparse.ArgumentParser,
> - namespace: argparse.Namespace,
> - values: Any,
> - option_string: str = None,
> - ) -> None:
> - if self.const is not None:
> - setattr(namespace, self.dest, self.const)
> - else:
> - setattr(namespace, self.dest, values)
> -
> - return _EnvironmentArgument
> -
>
> @dataclass(slots=True)
> class Settings:
> @@ -171,7 +102,7 @@ class Settings:
> #:
> compile_timeout: float = 1200
> #:
> - test_cases: list[str] = field(default_factory=list)
> + test_suites: list[TestSuiteConfig] = field(default_factory=list)
> #:
> re_run: int = 0
>
> @@ -180,6 +111,31 @@ class Settings:
>
>
> def _get_parser() -> argparse.ArgumentParser:
> + """Create the argument parser for DTS.
> +
> + Command line options take precedence over environment variables, which
> in turn take precedence
> + over default values.
> +
> + Returns:
> + argparse.ArgumentParser: The configured argument parser with defined
> options.
> + """
> +
> + def env_arg(env_var: str, default: Any) -> Any:
> + """A helper function augmenting the argparse with environment
> variables.
> +
> + If the supplied environment variable is defined, then the default
> value
> + of the argument is modified. This satisfies the priority order of
> + command line argument > environment variable > default value.
> +
> + Args:
> + env_var: Environment variable name.
> + default: Default value.
> +
> + Returns:
> + Environment variable or default value.
> + """
> + return os.environ.get(env_var) or default
> +
> parser = argparse.ArgumentParser(
> description="Run DPDK test suites. All options may be specified with
> the environment "
> "variables provided in brackets. Command line arguments have higher
> priority.",
> @@ -188,25 +144,23 @@ def _get_parser() -> argparse.ArgumentParser:
>
> parser.add_argument(
> "--config-file",
> - action=_env_arg("DTS_CFG_FILE"),
> - default=SETTINGS.config_file_path,
> + default=env_arg("DTS_CFG_FILE", SETTINGS.config_file_path),
> type=Path,
> - help="[DTS_CFG_FILE] configuration file that describes the test
> cases, SUTs and targets.",
> + help="[DTS_CFG_FILE] The configuration file that describes the test
> cases, "
> + "SUTs and targets.",
> )
>
> parser.add_argument(
> "--output-dir",
> "--output",
> - action=_env_arg("DTS_OUTPUT_DIR"),
> - default=SETTINGS.output_dir,
> + default=env_arg("DTS_OUTPUT_DIR", SETTINGS.output_dir),
> help="[DTS_OUTPUT_DIR] Output directory where DTS logs and results
> are saved.",
> )
>
> parser.add_argument(
> "-t",
> "--timeout",
> - action=_env_arg("DTS_TIMEOUT"),
> - default=SETTINGS.timeout,
> + default=env_arg("DTS_TIMEOUT", SETTINGS.timeout),
> type=float,
> help="[DTS_TIMEOUT] The default timeout for all DTS operations
> except for compiling DPDK.",
> )
> @@ -214,9 +168,8 @@ def _get_parser() -> argparse.ArgumentParser:
> parser.add_argument(
> "-v",
> "--verbose",
> - action=_env_arg("DTS_VERBOSE"),
> - default=SETTINGS.verbose,
> - const=True,
> + action="store_true",
> + default=env_arg("DTS_VERBOSE", SETTINGS.verbose),
> help="[DTS_VERBOSE] Specify to enable verbose output, logging all
> messages "
> "to the console.",
> )
> @@ -224,8 +177,8 @@ def _get_parser() -> argparse.ArgumentParser:
> parser.add_argument(
> "-s",
> "--skip-setup",
> - action=_env_arg("DTS_SKIP_SETUP"),
> - const=True,
> + action="store_true",
> + default=env_arg("DTS_SKIP_SETUP", SETTINGS.skip_setup),
> help="[DTS_SKIP_SETUP] Specify to skip all setup steps on SUT and TG
> nodes.",
> )
>
> @@ -233,8 +186,7 @@ def _get_parser() -> argparse.ArgumentParser:
> "--tarball",
> "--snapshot",
> "--git-ref",
> - action=_env_arg("DTS_DPDK_TARBALL"),
> - default=SETTINGS.dpdk_tarball_path,
> + default=env_arg("DTS_DPDK_TARBALL", SETTINGS.dpdk_tarball_path),
> type=Path,
> help="[DTS_DPDK_TARBALL] Path to DPDK source code tarball or a git
> commit ID, "
> "tag ID or tree ID to test. To test local changes, first commit
> them, "
> @@ -243,36 +195,71 @@ def _get_parser() -> argparse.ArgumentParser:
>
> parser.add_argument(
> "--compile-timeout",
> - action=_env_arg("DTS_COMPILE_TIMEOUT"),
> - default=SETTINGS.compile_timeout,
> + default=env_arg("DTS_COMPILE_TIMEOUT", SETTINGS.compile_timeout),
> type=float,
> help="[DTS_COMPILE_TIMEOUT] The timeout for compiling DPDK.",
> )
>
> parser.add_argument(
> - "--test-cases",
> - action=_env_arg("DTS_TESTCASES"),
> - default="",
> - help="[DTS_TESTCASES] Comma-separated list of test cases to
> execute.",
> + "--test-suite",
> + action="append",
> + nargs="+",
> + metavar=("TEST_SUITE", "TEST_CASES"),
> + default=env_arg("DTS_TEST_SUITES", SETTINGS.test_suites),
> + help="[DTS_TEST_SUITES] A list containing a test suite with test
> cases. "
> + "The first parameter is the test suite name, and the rest are test
> case names, "
> + "which are optional. May be specified multiple times. To specify
> multiple test suites in "
> + "the environment variable, join the lists with a comma. "
> + "Examples: "
> + "--test-suite suite case case --test-suite suite case ... | "
> + "DTS_TEST_SUITES='suite case case, suite case, ...' | "
> + "--test-suite suite --test-suite suite case ... | "
> + "DTS_TEST_SUITES='suite, suite case, ...'",
> )
>
> parser.add_argument(
> "--re-run",
> "--re_run",
> - action=_env_arg("DTS_RERUN"),
> - default=SETTINGS.re_run,
> + default=env_arg("DTS_RERUN", SETTINGS.re_run),
> type=int,
> help="[DTS_RERUN] Re-run each test case the specified number of
> times "
> - "if a test failure occurs",
> + "if a test failure occurs.",
> )
>
> return parser
>
>
> +def _process_test_suites(args: str | list[list[str]]) ->
> list[TestSuiteConfig]:
> + """Process the given argument to a list of :class:`TestSuiteConfig` to
> execute.
> +
> + Args:
> + args: The arguments to process. The args is a string from an
> environment variable
> + or a list of from the user input containing tests suites with
> tests cases,
> + each of which is a list of [test_suite, test_case, test_case,
> ...].
> +
> + Returns:
> + A list of test suite configurations to execute.
> + """
> + if isinstance(args, str):
> + # Environment variable in the form of "suite case case, suite case,
> suite, ..."
> + args = [suite_with_cases.split() for suite_with_cases in
> args.split(",")]
> +
> + test_suites_to_run = []
> + for suite_with_cases in args:
> + test_suites_to_run.append(
> + TestSuiteConfig(test_suite=suite_with_cases[0],
> test_cases=suite_with_cases[1:])
> + )
> +
> + return test_suites_to_run
> +
> +
> def get_settings() -> Settings:
> """Create new settings with inputs from the user.
>
> The inputs are taken from the command line and from environment
> variables.
> +
> + Returns:
> + The new settings object.
> """
> parsed_args = _get_parser().parse_args()
> return Settings(
> @@ -287,6 +274,6 @@ def get_settings() -> Settings:
> else Path(parsed_args.tarball)
> ),
> compile_timeout=parsed_args.compile_timeout,
> - test_cases=(parsed_args.test_cases.split(",") if
> parsed_args.test_cases else []),
> + test_suites=_process_test_suites(parsed_args.test_suite),
> re_run=parsed_args.re_run,
> )
> diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py
> index 365f80e21a..1957ea7328 100644
> --- a/dts/framework/test_suite.py
> +++ b/dts/framework/test_suite.py
> @@ -40,7 +40,7 @@ class TestSuite(object):
> and functional test cases (all other test cases).
>
> By default, all test cases will be executed. A list of testcase names
> may be specified
> - in the YAML test run configuration file and in the
> :option:`--test-cases` command line argument
> + in the YAML test run configuration file and in the
> :option:`--test-suite` command line argument
> or in the :envvar:`DTS_TESTCASES` environment variable to filter which
> test cases to run.
> The union of both lists will be used. Any unknown test cases from the
> latter lists
> will be silently ignored.
> --
> 2.34.1
>
