[v7,09/21] dts: test result docstring update
Checks
Commit Message
Format according to the Google format and PEP257, with slight
deviations.
Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
---
dts/framework/test_result.py | 292 ++++++++++++++++++++++++++++-------
1 file changed, 234 insertions(+), 58 deletions(-)
Comments
The only comments I had on this were a few places where I think attribute
sections should be class variables instead. I tried to mark all of the
places I saw it and it could be a difference where because of the way they
are subclassed they might do it differently but I'm unsure.
On Wed, Nov 15, 2023 at 8:12 AM Juraj Linkeš <juraj.linkes@pantheon.tech>
wrote:
> Format according to the Google format and PEP257, with slight
> deviations.
>
> Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> ---
> dts/framework/test_result.py | 292 ++++++++++++++++++++++++++++-------
> 1 file changed, 234 insertions(+), 58 deletions(-)
>
> diff --git a/dts/framework/test_result.py b/dts/framework/test_result.py
> index 603e18872c..05e210f6e7 100644
> --- a/dts/framework/test_result.py
> +++ b/dts/framework/test_result.py
> @@ -2,8 +2,25 @@
> # Copyright(c) 2023 PANTHEON.tech s.r.o.
> # Copyright(c) 2023 University of New Hampshire
>
> -"""
> -Generic result container and reporters
> +r"""Record and process DTS results.
> +
> +The results are recorded in a hierarchical manner:
> +
> + * :class:`DTSResult` contains
> + * :class:`ExecutionResult` contains
> + * :class:`BuildTargetResult` contains
> + * :class:`TestSuiteResult` contains
> + * :class:`TestCaseResult`
> +
> +Each result may contain multiple lower level results, e.g. there are
> multiple
> +:class:`TestSuiteResult`\s in a :class:`BuildTargetResult`.
> +The results have common parts, such as setup and teardown results,
> captured in :class:`BaseResult`,
> +which also defines some common behaviors in its methods.
> +
> +Each result class has its own idiosyncrasies which they implement in
> overridden methods.
> +
> +The :option:`--output` command line argument and the
> :envvar:`DTS_OUTPUT_DIR` environment
> +variable modify the directory where the files with results will be stored.
> """
>
> import os.path
> @@ -26,26 +43,34 @@
>
>
> class Result(Enum):
> - """
> - An Enum defining the possible states that
> - a setup, a teardown or a test case may end up in.
> - """
> + """The possible states that a setup, a teardown or a test case may
> end up in."""
>
> + #:
> PASS = auto()
> + #:
> FAIL = auto()
> + #:
> ERROR = auto()
> + #:
> SKIP = auto()
>
> def __bool__(self) -> bool:
> + """Only PASS is True."""
> return self is self.PASS
>
>
> class FixtureResult(object):
> - """
> - A record that stored the result of a setup or a teardown.
> - The default is FAIL because immediately after creating the object
> - the setup of the corresponding stage will be executed, which also
> guarantees
> - the execution of teardown.
> + """A record that stores the result of a setup or a teardown.
> +
> + FAIL is a sensible default since it prevents false positives
> + (which could happen if the default was PASS).
> +
> + Preventing false positives or other false results is preferable since
> a failure
> + is mostly likely to be investigated (the other false results may not
> be investigated at all).
> +
> + Attributes:
> + result: The associated result.
> + error: The error in case of a failure.
> """
>
I think the items in the attributes section should instead be "#:" because
they are class variables.
>
> result: Result
> @@ -56,21 +81,32 @@ def __init__(
> result: Result = Result.FAIL,
> error: Exception | None = None,
> ):
> + """Initialize the constructor with the fixture result and store a
> possible error.
> +
> + Args:
> + result: The result to store.
> + error: The error which happened when a failure occurred.
> + """
> self.result = result
> self.error = error
>
> def __bool__(self) -> bool:
> + """A wrapper around the stored :class:`Result`."""
> return bool(self.result)
>
>
> class Statistics(dict):
> - """
> - A helper class used to store the number of test cases by its result
> - along a few other basic information.
> - Using a dict provides a convenient way to format the data.
> + """How many test cases ended in which result state along some other
> basic information.
> +
> + Subclassing :class:`dict` provides a convenient way to format the
> data.
> """
>
> def __init__(self, dpdk_version: str | None):
> + """Extend the constructor with relevant keys.
> +
> + Args:
> + dpdk_version: The version of tested DPDK.
> + """
>
Should we maybe mark the "PASS RATE" and the "DPDK VERSION" as instance
variables of the class?
> super(Statistics, self).__init__()
> for result in Result:
> self[result.name] = 0
> @@ -78,8 +114,17 @@ def __init__(self, dpdk_version: str | None):
> self["DPDK VERSION"] = dpdk_version
>
> def __iadd__(self, other: Result) -> "Statistics":
> - """
> - Add a Result to the final count.
> + """Add a Result to the final count.
> +
> + Example:
> + stats: Statistics = Statistics() # empty Statistics
> + stats += Result.PASS # add a Result to `stats`
> +
> + Args:
> + other: The Result to add to this statistics object.
> +
> + Returns:
> + The modified statistics object.
> """
> self[other.name] += 1
> self["PASS RATE"] = (
> @@ -90,9 +135,7 @@ def __iadd__(self, other: Result) -> "Statistics":
> return self
>
> def __str__(self) -> str:
> - """
> - Provide a string representation of the data.
> - """
> + """Each line contains the formatted key = value pair."""
> stats_str = ""
> for key, value in self.items():
> stats_str += f"{key:<12} = {value}\n"
> @@ -102,10 +145,16 @@ def __str__(self) -> str:
>
>
> class BaseResult(object):
> - """
> - The Base class for all results. Stores the results of
> - the setup and teardown portions of the corresponding stage
> - and a list of results from each inner stage in _inner_results.
> + """Common data and behavior of DTS results.
> +
> + Stores the results of the setup and teardown portions of the
> corresponding stage.
> + The hierarchical nature of DTS results is captured recursively in an
> internal list.
> + A stage is each level in this particular hierarchy (pre-execution or
> the top-most level,
> + execution, build target, test suite and test case.)
> +
> + Attributes:
> + setup_result: The result of the setup of the particular stage.
> + teardown_result: The results of the teardown of the particular
> stage.
> """
>
I think this might be another case of the attributes should be marked as
class variables instead of instance variables.
>
> setup_result: FixtureResult
> @@ -113,15 +162,28 @@ class BaseResult(object):
> _inner_results: MutableSequence["BaseResult"]
>
> def __init__(self):
> + """Initialize the constructor."""
> self.setup_result = FixtureResult()
> self.teardown_result = FixtureResult()
> self._inner_results = []
>
> def update_setup(self, result: Result, error: Exception | None =
> None) -> None:
> + """Store the setup result.
> +
> + Args:
> + result: The result of the setup.
> + error: The error that occurred in case of a failure.
> + """
> self.setup_result.result = result
> self.setup_result.error = error
>
> def update_teardown(self, result: Result, error: Exception | None =
> None) -> None:
> + """Store the teardown result.
> +
> + Args:
> + result: The result of the teardown.
> + error: The error that occurred in case of a failure.
> + """
> self.teardown_result.result = result
> self.teardown_result.error = error
>
> @@ -141,27 +203,55 @@ def _get_inner_errors(self) -> list[Exception]:
> ]
>
> def get_errors(self) -> list[Exception]:
> + """Compile errors from the whole result hierarchy.
> +
> + Returns:
> + The errors from setup, teardown and all errors found in the
> whole result hierarchy.
> + """
> return self._get_setup_teardown_errors() +
> self._get_inner_errors()
>
> def add_stats(self, statistics: Statistics) -> None:
> + """Collate stats from the whole result hierarchy.
> +
> + Args:
> + statistics: The :class:`Statistics` object where the stats
> will be collated.
> + """
> for inner_result in self._inner_results:
> inner_result.add_stats(statistics)
>
>
> class TestCaseResult(BaseResult, FixtureResult):
> - """
> - The test case specific result.
> - Stores the result of the actual test case.
> - Also stores the test case name.
> + r"""The test case specific result.
> +
> + Stores the result of the actual test case. This is done by adding an
> extra superclass
> + in :class:`FixtureResult`. The setup and teardown results are
> :class:`FixtureResult`\s and
> + the class is itself a record of the test case.
> +
> + Attributes:
> + test_case_name: The test case name.
> """
>
>
Another spot where I think this should have a class variable comment.
> test_case_name: str
>
> def __init__(self, test_case_name: str):
> + """Extend the constructor with `test_case_name`.
> +
> + Args:
> + test_case_name: The test case's name.
> + """
> super(TestCaseResult, self).__init__()
> self.test_case_name = test_case_name
>
> def update(self, result: Result, error: Exception | None = None) ->
> None:
> + """Update the test case result.
> +
> + This updates the result of the test case itself and doesn't affect
> + the results of the setup and teardown steps in any way.
> +
> + Args:
> + result: The result of the test case.
> + error: The error that occurred in case of a failure.
> + """
> self.result = result
> self.error = error
>
> @@ -171,38 +261,66 @@ def _get_inner_errors(self) -> list[Exception]:
> return []
>
> def add_stats(self, statistics: Statistics) -> None:
> + r"""Add the test case result to statistics.
> +
> + The base method goes through the hierarchy recursively and this
> method is here to stop
> + the recursion, as the :class:`TestCaseResult`\s are the leaves of
> the hierarchy tree.
> +
> + Args:
> + statistics: The :class:`Statistics` object where the stats
> will be added.
> + """
> statistics += self.result
>
> def __bool__(self) -> bool:
> + """The test case passed only if setup, teardown and the test case
> itself passed."""
> return (
> bool(self.setup_result) and bool(self.teardown_result) and
> bool(self.result)
> )
>
>
> class TestSuiteResult(BaseResult):
> - """
> - The test suite specific result.
> - The _inner_results list stores results of test cases in a given test
> suite.
> - Also stores the test suite name.
> + """The test suite specific result.
> +
> + The internal list stores the results of all test cases in a given
> test suite.
> +
> + Attributes:
> + suite_name: The test suite name.
> """
>
>
I think this should also be a class variable.
> suite_name: str
>
> def __init__(self, suite_name: str):
> + """Extend the constructor with `suite_name`.
> +
> + Args:
> + suite_name: The test suite's name.
> + """
> super(TestSuiteResult, self).__init__()
> self.suite_name = suite_name
>
> def add_test_case(self, test_case_name: str) -> TestCaseResult:
> + """Add and return the inner result (test case).
> +
> + Returns:
> + The test case's result.
> + """
> test_case_result = TestCaseResult(test_case_name)
> self._inner_results.append(test_case_result)
> return test_case_result
>
>
> class BuildTargetResult(BaseResult):
> - """
> - The build target specific result.
> - The _inner_results list stores results of test suites in a given
> build target.
> - Also stores build target specifics, such as compiler used to build
> DPDK.
> + """The build target specific result.
> +
> + The internal list stores the results of all test suites in a given
> build target.
> +
> + Attributes:
> + arch: The DPDK build target architecture.
> + os: The DPDK build target operating system.
> + cpu: The DPDK build target CPU.
> + compiler: The DPDK build target compiler.
> + compiler_version: The DPDK build target compiler version.
> + dpdk_version: The built DPDK version.
> """
>
I think this should be broken into class variables as well.
>
> arch: Architecture
> @@ -213,6 +331,11 @@ class BuildTargetResult(BaseResult):
> dpdk_version: str | None
>
> def __init__(self, build_target: BuildTargetConfiguration):
> + """Extend the constructor with the `build_target`'s build target
> config.
> +
> + Args:
> + build_target: The build target's test run configuration.
> + """
> super(BuildTargetResult, self).__init__()
> self.arch = build_target.arch
> self.os = build_target.os
> @@ -222,20 +345,35 @@ def __init__(self, build_target:
> BuildTargetConfiguration):
> self.dpdk_version = None
>
> def add_build_target_info(self, versions: BuildTargetInfo) -> None:
> + """Add information about the build target gathered at runtime.
> +
> + Args:
> + versions: The additional information.
> + """
> self.compiler_version = versions.compiler_version
> self.dpdk_version = versions.dpdk_version
>
> def add_test_suite(self, test_suite_name: str) -> TestSuiteResult:
> + """Add and return the inner result (test suite).
> +
> + Returns:
> + The test suite's result.
> + """
> test_suite_result = TestSuiteResult(test_suite_name)
> self._inner_results.append(test_suite_result)
> return test_suite_result
>
>
> class ExecutionResult(BaseResult):
> - """
> - The execution specific result.
> - The _inner_results list stores results of build targets in a given
> execution.
> - Also stores the SUT node configuration.
> + """The execution specific result.
> +
> + The internal list stores the results of all build targets in a given
> execution.
> +
> + Attributes:
> + sut_node: The SUT node used in the execution.
> + sut_os_name: The operating system of the SUT node.
> + sut_os_version: The operating system version of the SUT node.
> + sut_kernel_version: The operating system kernel version of the
> SUT node.
> """
>
>
I think these should be class variables as well.
> sut_node: NodeConfiguration
> @@ -244,36 +382,55 @@ class ExecutionResult(BaseResult):
> sut_kernel_version: str
>
> def __init__(self, sut_node: NodeConfiguration):
> + """Extend the constructor with the `sut_node`'s config.
> +
> + Args:
> + sut_node: The SUT node's test run configuration used in the
> execution.
> + """
> super(ExecutionResult, self).__init__()
> self.sut_node = sut_node
>
> def add_build_target(
> self, build_target: BuildTargetConfiguration
> ) -> BuildTargetResult:
> + """Add and return the inner result (build target).
> +
> + Args:
> + build_target: The build target's test run configuration.
> +
> + Returns:
> + The build target's result.
> + """
> build_target_result = BuildTargetResult(build_target)
> self._inner_results.append(build_target_result)
> return build_target_result
>
> def add_sut_info(self, sut_info: NodeInfo) -> None:
> + """Add SUT information gathered at runtime.
> +
> + Args:
> + sut_info: The additional SUT node information.
> + """
> self.sut_os_name = sut_info.os_name
> self.sut_os_version = sut_info.os_version
> self.sut_kernel_version = sut_info.kernel_version
>
>
> class DTSResult(BaseResult):
> - """
> - Stores environment information and test results from a DTS run, which
> are:
> - * Execution level information, such as SUT and TG hardware.
> - * Build target level information, such as compiler, target OS and cpu.
> - * Test suite results.
> - * All errors that are caught and recorded during DTS execution.
> + """Stores environment information and test results from a DTS run.
>
> - The information is stored in nested objects.
> + * Execution level information, such as testbed and the test suite
> list,
> + * Build target level information, such as compiler, target OS and
> cpu,
> + * Test suite and test case results,
> + * All errors that are caught and recorded during DTS execution.
>
> - The class is capable of computing the return code used to exit DTS
> with
> - from the stored error.
> + The information is stored hierarchically. This is the first level of
> the hierarchy
> + and as such is where the data form the whole hierarchy is collated or
> processed.
>
> - It also provides a brief statistical summary of passed/failed test
> cases.
> + The internal list stores the results of all executions.
> +
> + Attributes:
> + dpdk_version: The DPDK version to record.
> """
>
>
I think this should be a class variable as well.
> dpdk_version: str | None
> @@ -284,6 +441,11 @@ class DTSResult(BaseResult):
> _stats_filename: str
>
> def __init__(self, logger: DTSLOG):
> + """Extend the constructor with top-level specifics.
> +
> + Args:
> + logger: The logger instance the whole result will use.
> + """
> super(DTSResult, self).__init__()
> self.dpdk_version = None
> self._logger = logger
> @@ -293,21 +455,33 @@ def __init__(self, logger: DTSLOG):
> self._stats_filename = os.path.join(SETTINGS.output_dir,
> "statistics.txt")
>
> def add_execution(self, sut_node: NodeConfiguration) ->
> ExecutionResult:
> + """Add and return the inner result (execution).
> +
> + Args:
> + sut_node: The SUT node's test run configuration.
> +
> + Returns:
> + The execution's result.
> + """
> execution_result = ExecutionResult(sut_node)
> self._inner_results.append(execution_result)
> return execution_result
>
> def add_error(self, error: Exception) -> None:
> + """Record an error that occurred outside any execution.
> +
> + Args:
> + error: The exception to record.
> + """
> self._errors.append(error)
>
> def process(self) -> None:
> - """
> - Process the data after a DTS run.
> - The data is added to nested objects during runtime and this
> parent object
> - is not updated at that time. This requires us to process the
> nested data
> - after it's all been gathered.
> + """Process the data after a whole DTS run.
> +
> + The data is added to inner objects during runtime and this object
> is not updated
> + at that time. This requires us to process the inner data after
> it's all been gathered.
>
> - The processing gathers all errors and the result statistics of
> test cases.
> + The processing gathers all errors and the statistics of test case
> results.
> """
> self._errors += self.get_errors()
> if self._errors and self._logger:
> @@ -321,8 +495,10 @@ def process(self) -> None:
> stats_file.write(str(self._stats_result))
>
> def get_return_code(self) -> int:
> - """
> - Go through all stored Exceptions and return the highest error
> code found.
> + """Go through all stored Exceptions and return the final DTS
> error code.
> +
> + Returns:
> + The highest error code found.
> """
> for error in self._errors:
> error_return_code = ErrorSeverity.GENERIC_ERR
> --
> 2.34.1
>
>
On Thu, Nov 16, 2023 at 11:47 PM Jeremy Spewock <jspewock@iol.unh.edu> wrote:
>
> The only comments I had on this were a few places where I think attribute sections should be class variables instead. I tried to mark all of the places I saw it and it could be a difference where because of the way they are subclassed they might do it differently but I'm unsure.
>
> On Wed, Nov 15, 2023 at 8:12 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote:
>>
>> Format according to the Google format and PEP257, with slight
>> deviations.
>>
>> Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
>> ---
>> dts/framework/test_result.py | 292 ++++++++++++++++++++++++++++-------
>> 1 file changed, 234 insertions(+), 58 deletions(-)
>>
>> diff --git a/dts/framework/test_result.py b/dts/framework/test_result.py
>> index 603e18872c..05e210f6e7 100644
>> --- a/dts/framework/test_result.py
>> +++ b/dts/framework/test_result.py
>> @@ -2,8 +2,25 @@
>> # Copyright(c) 2023 PANTHEON.tech s.r.o.
>> # Copyright(c) 2023 University of New Hampshire
>>
>> -"""
>> -Generic result container and reporters
>> +r"""Record and process DTS results.
>> +
>> +The results are recorded in a hierarchical manner:
>> +
>> + * :class:`DTSResult` contains
>> + * :class:`ExecutionResult` contains
>> + * :class:`BuildTargetResult` contains
>> + * :class:`TestSuiteResult` contains
>> + * :class:`TestCaseResult`
>> +
>> +Each result may contain multiple lower level results, e.g. there are multiple
>> +:class:`TestSuiteResult`\s in a :class:`BuildTargetResult`.
>> +The results have common parts, such as setup and teardown results, captured in :class:`BaseResult`,
>> +which also defines some common behaviors in its methods.
>> +
>> +Each result class has its own idiosyncrasies which they implement in overridden methods.
>> +
>> +The :option:`--output` command line argument and the :envvar:`DTS_OUTPUT_DIR` environment
>> +variable modify the directory where the files with results will be stored.
>> """
>>
>> import os.path
>> @@ -26,26 +43,34 @@
>>
>>
>> class Result(Enum):
>> - """
>> - An Enum defining the possible states that
>> - a setup, a teardown or a test case may end up in.
>> - """
>> + """The possible states that a setup, a teardown or a test case may end up in."""
>>
>> + #:
>> PASS = auto()
>> + #:
>> FAIL = auto()
>> + #:
>> ERROR = auto()
>> + #:
>> SKIP = auto()
>>
>> def __bool__(self) -> bool:
>> + """Only PASS is True."""
>> return self is self.PASS
>>
>>
>> class FixtureResult(object):
>> - """
>> - A record that stored the result of a setup or a teardown.
>> - The default is FAIL because immediately after creating the object
>> - the setup of the corresponding stage will be executed, which also guarantees
>> - the execution of teardown.
>> + """A record that stores the result of a setup or a teardown.
>> +
>> + FAIL is a sensible default since it prevents false positives
>> + (which could happen if the default was PASS).
>> +
>> + Preventing false positives or other false results is preferable since a failure
>> + is mostly likely to be investigated (the other false results may not be investigated at all).
>> +
>> + Attributes:
>> + result: The associated result.
>> + error: The error in case of a failure.
>> """
>
>
> I think the items in the attributes section should instead be "#:" because they are class variables.
>
Making these class variables would make the value the same for all
instances, of which there are plenty. Why do you think these should be
class variables?
>>
>>
>> result: Result
>> @@ -56,21 +81,32 @@ def __init__(
>> result: Result = Result.FAIL,
>> error: Exception | None = None,
>> ):
>> + """Initialize the constructor with the fixture result and store a possible error.
>> +
>> + Args:
>> + result: The result to store.
>> + error: The error which happened when a failure occurred.
>> + """
>> self.result = result
>> self.error = error
>>
>> def __bool__(self) -> bool:
>> + """A wrapper around the stored :class:`Result`."""
>> return bool(self.result)
>>
>>
>> class Statistics(dict):
>> - """
>> - A helper class used to store the number of test cases by its result
>> - along a few other basic information.
>> - Using a dict provides a convenient way to format the data.
>> + """How many test cases ended in which result state along some other basic information.
>> +
>> + Subclassing :class:`dict` provides a convenient way to format the data.
>> """
>>
>> def __init__(self, dpdk_version: str | None):
>> + """Extend the constructor with relevant keys.
>> +
>> + Args:
>> + dpdk_version: The version of tested DPDK.
>> + """
>
>
> Should we maybe mark the "PASS RATE" and the "DPDK VERSION" as instance variables of the class?
>
This is a dict, so these won't work as instance variables, but it
makes sense to document these keys, so I'll add that.
>>
>> super(Statistics, self).__init__()
>> for result in Result:
>> self[result.name] = 0
>> @@ -78,8 +114,17 @@ def __init__(self, dpdk_version: str | None):
>> self["DPDK VERSION"] = dpdk_version
>>
>> def __iadd__(self, other: Result) -> "Statistics":
>> - """
>> - Add a Result to the final count.
>> + """Add a Result to the final count.
>> +
>> + Example:
>> + stats: Statistics = Statistics() # empty Statistics
>> + stats += Result.PASS # add a Result to `stats`
>> +
>> + Args:
>> + other: The Result to add to this statistics object.
>> +
>> + Returns:
>> + The modified statistics object.
>> """
>> self[other.name] += 1
>> self["PASS RATE"] = (
>> @@ -90,9 +135,7 @@ def __iadd__(self, other: Result) -> "Statistics":
>> return self
>>
>> def __str__(self) -> str:
>> - """
>> - Provide a string representation of the data.
>> - """
>> + """Each line contains the formatted key = value pair."""
>> stats_str = ""
>> for key, value in self.items():
>> stats_str += f"{key:<12} = {value}\n"
>> @@ -102,10 +145,16 @@ def __str__(self) -> str:
>>
>>
>> class BaseResult(object):
>> - """
>> - The Base class for all results. Stores the results of
>> - the setup and teardown portions of the corresponding stage
>> - and a list of results from each inner stage in _inner_results.
>> + """Common data and behavior of DTS results.
>> +
>> + Stores the results of the setup and teardown portions of the corresponding stage.
>> + The hierarchical nature of DTS results is captured recursively in an internal list.
>> + A stage is each level in this particular hierarchy (pre-execution or the top-most level,
>> + execution, build target, test suite and test case.)
>> +
>> + Attributes:
>> + setup_result: The result of the setup of the particular stage.
>> + teardown_result: The results of the teardown of the particular stage.
>> """
>
>
> I think this might be another case of the attributes should be marked as class variables instead of instance variables.
>
This is the same as in FixtureResult. For example, there could be
multiple build targets with different results.
>>
>>
>> setup_result: FixtureResult
>> @@ -113,15 +162,28 @@ class BaseResult(object):
>> _inner_results: MutableSequence["BaseResult"]
>>
>> def __init__(self):
>> + """Initialize the constructor."""
>> self.setup_result = FixtureResult()
>> self.teardown_result = FixtureResult()
>> self._inner_results = []
>>
>> def update_setup(self, result: Result, error: Exception | None = None) -> None:
>> + """Store the setup result.
>> +
>> + Args:
>> + result: The result of the setup.
>> + error: The error that occurred in case of a failure.
>> + """
>> self.setup_result.result = result
>> self.setup_result.error = error
>>
>> def update_teardown(self, result: Result, error: Exception | None = None) -> None:
>> + """Store the teardown result.
>> +
>> + Args:
>> + result: The result of the teardown.
>> + error: The error that occurred in case of a failure.
>> + """
>> self.teardown_result.result = result
>> self.teardown_result.error = error
>>
>> @@ -141,27 +203,55 @@ def _get_inner_errors(self) -> list[Exception]:
>> ]
>>
>> def get_errors(self) -> list[Exception]:
>> + """Compile errors from the whole result hierarchy.
>> +
>> + Returns:
>> + The errors from setup, teardown and all errors found in the whole result hierarchy.
>> + """
>> return self._get_setup_teardown_errors() + self._get_inner_errors()
>>
>> def add_stats(self, statistics: Statistics) -> None:
>> + """Collate stats from the whole result hierarchy.
>> +
>> + Args:
>> + statistics: The :class:`Statistics` object where the stats will be collated.
>> + """
>> for inner_result in self._inner_results:
>> inner_result.add_stats(statistics)
>>
>>
>> class TestCaseResult(BaseResult, FixtureResult):
>> - """
>> - The test case specific result.
>> - Stores the result of the actual test case.
>> - Also stores the test case name.
>> + r"""The test case specific result.
>> +
>> + Stores the result of the actual test case. This is done by adding an extra superclass
>> + in :class:`FixtureResult`. The setup and teardown results are :class:`FixtureResult`\s and
>> + the class is itself a record of the test case.
>> +
>> + Attributes:
>> + test_case_name: The test case name.
>> """
>>
>
> Another spot where I think this should have a class variable comment.
>
>>
>> test_case_name: str
>>
>> def __init__(self, test_case_name: str):
>> + """Extend the constructor with `test_case_name`.
>> +
>> + Args:
>> + test_case_name: The test case's name.
>> + """
>> super(TestCaseResult, self).__init__()
>> self.test_case_name = test_case_name
>>
>> def update(self, result: Result, error: Exception | None = None) -> None:
>> + """Update the test case result.
>> +
>> + This updates the result of the test case itself and doesn't affect
>> + the results of the setup and teardown steps in any way.
>> +
>> + Args:
>> + result: The result of the test case.
>> + error: The error that occurred in case of a failure.
>> + """
>> self.result = result
>> self.error = error
>>
>> @@ -171,38 +261,66 @@ def _get_inner_errors(self) -> list[Exception]:
>> return []
>>
>> def add_stats(self, statistics: Statistics) -> None:
>> + r"""Add the test case result to statistics.
>> +
>> + The base method goes through the hierarchy recursively and this method is here to stop
>> + the recursion, as the :class:`TestCaseResult`\s are the leaves of the hierarchy tree.
>> +
>> + Args:
>> + statistics: The :class:`Statistics` object where the stats will be added.
>> + """
>> statistics += self.result
>>
>> def __bool__(self) -> bool:
>> + """The test case passed only if setup, teardown and the test case itself passed."""
>> return (
>> bool(self.setup_result) and bool(self.teardown_result) and bool(self.result)
>> )
>>
>>
>> class TestSuiteResult(BaseResult):
>> - """
>> - The test suite specific result.
>> - The _inner_results list stores results of test cases in a given test suite.
>> - Also stores the test suite name.
>> + """The test suite specific result.
>> +
>> + The internal list stores the results of all test cases in a given test suite.
>> +
>> + Attributes:
>> + suite_name: The test suite name.
>> """
>>
>
> I think this should also be a class variable.
>
>
>>
>> suite_name: str
>>
>> def __init__(self, suite_name: str):
>> + """Extend the constructor with `suite_name`.
>> +
>> + Args:
>> + suite_name: The test suite's name.
>> + """
>> super(TestSuiteResult, self).__init__()
>> self.suite_name = suite_name
>>
>> def add_test_case(self, test_case_name: str) -> TestCaseResult:
>> + """Add and return the inner result (test case).
>> +
>> + Returns:
>> + The test case's result.
>> + """
>> test_case_result = TestCaseResult(test_case_name)
>> self._inner_results.append(test_case_result)
>> return test_case_result
>>
>>
>> class BuildTargetResult(BaseResult):
>> - """
>> - The build target specific result.
>> - The _inner_results list stores results of test suites in a given build target.
>> - Also stores build target specifics, such as compiler used to build DPDK.
>> + """The build target specific result.
>> +
>> + The internal list stores the results of all test suites in a given build target.
>> +
>> + Attributes:
>> + arch: The DPDK build target architecture.
>> + os: The DPDK build target operating system.
>> + cpu: The DPDK build target CPU.
>> + compiler: The DPDK build target compiler.
>> + compiler_version: The DPDK build target compiler version.
>> + dpdk_version: The built DPDK version.
>> """
>
>
> I think this should be broken into class variables as well.
>
>>
>>
>> arch: Architecture
>> @@ -213,6 +331,11 @@ class BuildTargetResult(BaseResult):
>> dpdk_version: str | None
>>
>> def __init__(self, build_target: BuildTargetConfiguration):
>> + """Extend the constructor with the `build_target`'s build target config.
>> +
>> + Args:
>> + build_target: The build target's test run configuration.
>> + """
>> super(BuildTargetResult, self).__init__()
>> self.arch = build_target.arch
>> self.os = build_target.os
>> @@ -222,20 +345,35 @@ def __init__(self, build_target: BuildTargetConfiguration):
>> self.dpdk_version = None
>>
>> def add_build_target_info(self, versions: BuildTargetInfo) -> None:
>> + """Add information about the build target gathered at runtime.
>> +
>> + Args:
>> + versions: The additional information.
>> + """
>> self.compiler_version = versions.compiler_version
>> self.dpdk_version = versions.dpdk_version
>>
>> def add_test_suite(self, test_suite_name: str) -> TestSuiteResult:
>> + """Add and return the inner result (test suite).
>> +
>> + Returns:
>> + The test suite's result.
>> + """
>> test_suite_result = TestSuiteResult(test_suite_name)
>> self._inner_results.append(test_suite_result)
>> return test_suite_result
>>
>>
>> class ExecutionResult(BaseResult):
>> - """
>> - The execution specific result.
>> - The _inner_results list stores results of build targets in a given execution.
>> - Also stores the SUT node configuration.
>> + """The execution specific result.
>> +
>> + The internal list stores the results of all build targets in a given execution.
>> +
>> + Attributes:
>> + sut_node: The SUT node used in the execution.
>> + sut_os_name: The operating system of the SUT node.
>> + sut_os_version: The operating system version of the SUT node.
>> + sut_kernel_version: The operating system kernel version of the SUT node.
>> """
>>
>
> I think these should be class variables as well.
>
>>
>> sut_node: NodeConfiguration
>> @@ -244,36 +382,55 @@ class ExecutionResult(BaseResult):
>> sut_kernel_version: str
>>
>> def __init__(self, sut_node: NodeConfiguration):
>> + """Extend the constructor with the `sut_node`'s config.
>> +
>> + Args:
>> + sut_node: The SUT node's test run configuration used in the execution.
>> + """
>> super(ExecutionResult, self).__init__()
>> self.sut_node = sut_node
>>
>> def add_build_target(
>> self, build_target: BuildTargetConfiguration
>> ) -> BuildTargetResult:
>> + """Add and return the inner result (build target).
>> +
>> + Args:
>> + build_target: The build target's test run configuration.
>> +
>> + Returns:
>> + The build target's result.
>> + """
>> build_target_result = BuildTargetResult(build_target)
>> self._inner_results.append(build_target_result)
>> return build_target_result
>>
>> def add_sut_info(self, sut_info: NodeInfo) -> None:
>> + """Add SUT information gathered at runtime.
>> +
>> + Args:
>> + sut_info: The additional SUT node information.
>> + """
>> self.sut_os_name = sut_info.os_name
>> self.sut_os_version = sut_info.os_version
>> self.sut_kernel_version = sut_info.kernel_version
>>
>>
>> class DTSResult(BaseResult):
>> - """
>> - Stores environment information and test results from a DTS run, which are:
>> - * Execution level information, such as SUT and TG hardware.
>> - * Build target level information, such as compiler, target OS and cpu.
>> - * Test suite results.
>> - * All errors that are caught and recorded during DTS execution.
>> + """Stores environment information and test results from a DTS run.
>>
>> - The information is stored in nested objects.
>> + * Execution level information, such as testbed and the test suite list,
>> + * Build target level information, such as compiler, target OS and cpu,
>> + * Test suite and test case results,
>> + * All errors that are caught and recorded during DTS execution.
>>
>> - The class is capable of computing the return code used to exit DTS with
>> - from the stored error.
>> + The information is stored hierarchically. This is the first level of the hierarchy
>> + and as such is where the data form the whole hierarchy is collated or processed.
>>
>> - It also provides a brief statistical summary of passed/failed test cases.
>> + The internal list stores the results of all executions.
>> +
>> + Attributes:
>> + dpdk_version: The DPDK version to record.
>> """
>>
>
> I think this should be a class variable as well.
>
This is the only place where making this a class variable would work,
but I don't see a reason for it. An instance variable works just as
well.
>>
>> dpdk_version: str | None
>> @@ -284,6 +441,11 @@ class DTSResult(BaseResult):
>> _stats_filename: str
>>
>> def __init__(self, logger: DTSLOG):
>> + """Extend the constructor with top-level specifics.
>> +
>> + Args:
>> + logger: The logger instance the whole result will use.
>> + """
>> super(DTSResult, self).__init__()
>> self.dpdk_version = None
>> self._logger = logger
>> @@ -293,21 +455,33 @@ def __init__(self, logger: DTSLOG):
>> self._stats_filename = os.path.join(SETTINGS.output_dir, "statistics.txt")
>>
>> def add_execution(self, sut_node: NodeConfiguration) -> ExecutionResult:
>> + """Add and return the inner result (execution).
>> +
>> + Args:
>> + sut_node: The SUT node's test run configuration.
>> +
>> + Returns:
>> + The execution's result.
>> + """
>> execution_result = ExecutionResult(sut_node)
>> self._inner_results.append(execution_result)
>> return execution_result
>>
>> def add_error(self, error: Exception) -> None:
>> + """Record an error that occurred outside any execution.
>> +
>> + Args:
>> + error: The exception to record.
>> + """
>> self._errors.append(error)
>>
>> def process(self) -> None:
>> - """
>> - Process the data after a DTS run.
>> - The data is added to nested objects during runtime and this parent object
>> - is not updated at that time. This requires us to process the nested data
>> - after it's all been gathered.
>> + """Process the data after a whole DTS run.
>> +
>> + The data is added to inner objects during runtime and this object is not updated
>> + at that time. This requires us to process the inner data after it's all been gathered.
>>
>> - The processing gathers all errors and the result statistics of test cases.
>> + The processing gathers all errors and the statistics of test case results.
>> """
>> self._errors += self.get_errors()
>> if self._errors and self._logger:
>> @@ -321,8 +495,10 @@ def process(self) -> None:
>> stats_file.write(str(self._stats_result))
>>
>> def get_return_code(self) -> int:
>> - """
>> - Go through all stored Exceptions and return the highest error code found.
>> + """Go through all stored Exceptions and return the final DTS error code.
>> +
>> + Returns:
>> + The highest error code found.
>> """
>> for error in self._errors:
>> error_return_code = ErrorSeverity.GENERIC_ERR
>> --
>> 2.34.1
>>
On Mon, Nov 20, 2023 at 11:33 AM Juraj Linkeš <juraj.linkes@pantheon.tech>
wrote:
> On Thu, Nov 16, 2023 at 11:47 PM Jeremy Spewock <jspewock@iol.unh.edu>
> wrote:
> >
> > The only comments I had on this were a few places where I think
> attribute sections should be class variables instead. I tried to mark all
> of the places I saw it and it could be a difference where because of the
> way they are subclassed they might do it differently but I'm unsure.
> >
> > On Wed, Nov 15, 2023 at 8:12 AM Juraj Linkeš <juraj.linkes@pantheon.tech>
> wrote:
> >>
> >> Format according to the Google format and PEP257, with slight
> >> deviations.
> >>
> >> Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> >> ---
> >> dts/framework/test_result.py | 292 ++++++++++++++++++++++++++++-------
> >> 1 file changed, 234 insertions(+), 58 deletions(-)
> >>
> >> diff --git a/dts/framework/test_result.py b/dts/framework/test_result.py
> >> index 603e18872c..05e210f6e7 100644
> >> --- a/dts/framework/test_result.py
> >> +++ b/dts/framework/test_result.py
> >> @@ -2,8 +2,25 @@
> >> # Copyright(c) 2023 PANTHEON.tech s.r.o.
> >> # Copyright(c) 2023 University of New Hampshire
> >>
> >> -"""
> >> -Generic result container and reporters
> >> +r"""Record and process DTS results.
> >> +
> >> +The results are recorded in a hierarchical manner:
> >> +
> >> + * :class:`DTSResult` contains
> >> + * :class:`ExecutionResult` contains
> >> + * :class:`BuildTargetResult` contains
> >> + * :class:`TestSuiteResult` contains
> >> + * :class:`TestCaseResult`
> >> +
> >> +Each result may contain multiple lower level results, e.g. there are
> multiple
> >> +:class:`TestSuiteResult`\s in a :class:`BuildTargetResult`.
> >> +The results have common parts, such as setup and teardown results,
> captured in :class:`BaseResult`,
> >> +which also defines some common behaviors in its methods.
> >> +
> >> +Each result class has its own idiosyncrasies which they implement in
> overridden methods.
> >> +
> >> +The :option:`--output` command line argument and the
> :envvar:`DTS_OUTPUT_DIR` environment
> >> +variable modify the directory where the files with results will be
> stored.
> >> """
> >>
> >> import os.path
> >> @@ -26,26 +43,34 @@
> >>
> >>
> >> class Result(Enum):
> >> - """
> >> - An Enum defining the possible states that
> >> - a setup, a teardown or a test case may end up in.
> >> - """
> >> + """The possible states that a setup, a teardown or a test case may
> end up in."""
> >>
> >> + #:
> >> PASS = auto()
> >> + #:
> >> FAIL = auto()
> >> + #:
> >> ERROR = auto()
> >> + #:
> >> SKIP = auto()
> >>
> >> def __bool__(self) -> bool:
> >> + """Only PASS is True."""
> >> return self is self.PASS
> >>
> >>
> >> class FixtureResult(object):
> >> - """
> >> - A record that stored the result of a setup or a teardown.
> >> - The default is FAIL because immediately after creating the object
> >> - the setup of the corresponding stage will be executed, which also
> guarantees
> >> - the execution of teardown.
> >> + """A record that stores the result of a setup or a teardown.
> >> +
> >> + FAIL is a sensible default since it prevents false positives
> >> + (which could happen if the default was PASS).
> >> +
> >> + Preventing false positives or other false results is preferable
> since a failure
> >> + is mostly likely to be investigated (the other false results may
> not be investigated at all).
> >> +
> >> + Attributes:
> >> + result: The associated result.
> >> + error: The error in case of a failure.
> >> """
> >
> >
> > I think the items in the attributes section should instead be "#:"
> because they are class variables.
> >
>
> Making these class variables would make the value the same for all
> instances, of which there are plenty. Why do you think these should be
> class variables?
>
That explanation makes more sense. I guess I was thinking of class
variables as anything we statically define as part of the class (i.e., like
we say the class will always have a `result` and an `error` attribute), but
I could have just been mistaken. Using the definition of instance variables
as they can differ between instances I agree makes this comment and the
other ones you touched on obsolete.
>
> >>
> >>
> >> result: Result
> >> @@ -56,21 +81,32 @@ def __init__(
> >> result: Result = Result.FAIL,
> >> error: Exception | None = None,
> >> ):
> >> + """Initialize the constructor with the fixture result and
> store a possible error.
> >> +
> >> + Args:
> >> + result: The result to store.
> >> + error: The error which happened when a failure occurred.
> >> + """
> >> self.result = result
> >> self.error = error
> >>
> >> def __bool__(self) -> bool:
> >> + """A wrapper around the stored :class:`Result`."""
> >> return bool(self.result)
> >>
> >>
> >> class Statistics(dict):
> >> - """
> >> - A helper class used to store the number of test cases by its result
> >> - along a few other basic information.
> >> - Using a dict provides a convenient way to format the data.
> >> + """How many test cases ended in which result state along some
> other basic information.
> >> +
> >> + Subclassing :class:`dict` provides a convenient way to format the
> data.
> >> """
> >>
> >> def __init__(self, dpdk_version: str | None):
> >> + """Extend the constructor with relevant keys.
> >> +
> >> + Args:
> >> + dpdk_version: The version of tested DPDK.
> >> + """
> >
> >
> > Should we maybe mark the "PASS RATE" and the "DPDK VERSION" as instance
> variables of the class?
> >
>
> This is a dict, so these won't work as instance variables, but it
> makes sense to document these keys, so I'll add that.
>
> >>
> >> super(Statistics, self).__init__()
> >> for result in Result:
> >> self[result.name] = 0
> >> @@ -78,8 +114,17 @@ def __init__(self, dpdk_version: str | None):
> >> self["DPDK VERSION"] = dpdk_version
> >>
> >> def __iadd__(self, other: Result) -> "Statistics":
> >> - """
> >> - Add a Result to the final count.
> >> + """Add a Result to the final count.
> >> +
> >> + Example:
> >> + stats: Statistics = Statistics() # empty Statistics
> >> + stats += Result.PASS # add a Result to `stats`
> >> +
> >> + Args:
> >> + other: The Result to add to this statistics object.
> >> +
> >> + Returns:
> >> + The modified statistics object.
> >> """
> >> self[other.name] += 1
> >> self["PASS RATE"] = (
> >> @@ -90,9 +135,7 @@ def __iadd__(self, other: Result) -> "Statistics":
> >> return self
> >>
> >> def __str__(self) -> str:
> >> - """
> >> - Provide a string representation of the data.
> >> - """
> >> + """Each line contains the formatted key = value pair."""
> >> stats_str = ""
> >> for key, value in self.items():
> >> stats_str += f"{key:<12} = {value}\n"
> >> @@ -102,10 +145,16 @@ def __str__(self) -> str:
> >>
> >>
> >> class BaseResult(object):
> >> - """
> >> - The Base class for all results. Stores the results of
> >> - the setup and teardown portions of the corresponding stage
> >> - and a list of results from each inner stage in _inner_results.
> >> + """Common data and behavior of DTS results.
> >> +
> >> + Stores the results of the setup and teardown portions of the
> corresponding stage.
> >> + The hierarchical nature of DTS results is captured recursively in
> an internal list.
> >> + A stage is each level in this particular hierarchy (pre-execution
> or the top-most level,
> >> + execution, build target, test suite and test case.)
> >> +
> >> + Attributes:
> >> + setup_result: The result of the setup of the particular stage.
> >> + teardown_result: The results of the teardown of the particular
> stage.
> >> """
> >
> >
> > I think this might be another case of the attributes should be marked as
> class variables instead of instance variables.
> >
>
> This is the same as in FixtureResult. For example, there could be
> multiple build targets with different results.
>
> >>
> >>
> >> setup_result: FixtureResult
> >> @@ -113,15 +162,28 @@ class BaseResult(object):
> >> _inner_results: MutableSequence["BaseResult"]
> >>
> >> def __init__(self):
> >> + """Initialize the constructor."""
> >> self.setup_result = FixtureResult()
> >> self.teardown_result = FixtureResult()
> >> self._inner_results = []
> >>
> >> def update_setup(self, result: Result, error: Exception | None =
> None) -> None:
> >> + """Store the setup result.
> >> +
> >> + Args:
> >> + result: The result of the setup.
> >> + error: The error that occurred in case of a failure.
> >> + """
> >> self.setup_result.result = result
> >> self.setup_result.error = error
> >>
> >> def update_teardown(self, result: Result, error: Exception | None
> = None) -> None:
> >> + """Store the teardown result.
> >> +
> >> + Args:
> >> + result: The result of the teardown.
> >> + error: The error that occurred in case of a failure.
> >> + """
> >> self.teardown_result.result = result
> >> self.teardown_result.error = error
> >>
> >> @@ -141,27 +203,55 @@ def _get_inner_errors(self) -> list[Exception]:
> >> ]
> >>
> >> def get_errors(self) -> list[Exception]:
> >> + """Compile errors from the whole result hierarchy.
> >> +
> >> + Returns:
> >> + The errors from setup, teardown and all errors found in
> the whole result hierarchy.
> >> + """
> >> return self._get_setup_teardown_errors() +
> self._get_inner_errors()
> >>
> >> def add_stats(self, statistics: Statistics) -> None:
> >> + """Collate stats from the whole result hierarchy.
> >> +
> >> + Args:
> >> + statistics: The :class:`Statistics` object where the stats
> will be collated.
> >> + """
> >> for inner_result in self._inner_results:
> >> inner_result.add_stats(statistics)
> >>
> >>
> >> class TestCaseResult(BaseResult, FixtureResult):
> >> - """
> >> - The test case specific result.
> >> - Stores the result of the actual test case.
> >> - Also stores the test case name.
> >> + r"""The test case specific result.
> >> +
> >> + Stores the result of the actual test case. This is done by adding
> an extra superclass
> >> + in :class:`FixtureResult`. The setup and teardown results are
> :class:`FixtureResult`\s and
> >> + the class is itself a record of the test case.
> >> +
> >> + Attributes:
> >> + test_case_name: The test case name.
> >> """
> >>
> >
> > Another spot where I think this should have a class variable comment.
> >
> >>
> >> test_case_name: str
> >>
> >> def __init__(self, test_case_name: str):
> >> + """Extend the constructor with `test_case_name`.
> >> +
> >> + Args:
> >> + test_case_name: The test case's name.
> >> + """
> >> super(TestCaseResult, self).__init__()
> >> self.test_case_name = test_case_name
> >>
> >> def update(self, result: Result, error: Exception | None = None)
> -> None:
> >> + """Update the test case result.
> >> +
> >> + This updates the result of the test case itself and doesn't
> affect
> >> + the results of the setup and teardown steps in any way.
> >> +
> >> + Args:
> >> + result: The result of the test case.
> >> + error: The error that occurred in case of a failure.
> >> + """
> >> self.result = result
> >> self.error = error
> >>
> >> @@ -171,38 +261,66 @@ def _get_inner_errors(self) -> list[Exception]:
> >> return []
> >>
> >> def add_stats(self, statistics: Statistics) -> None:
> >> + r"""Add the test case result to statistics.
> >> +
> >> + The base method goes through the hierarchy recursively and
> this method is here to stop
> >> + the recursion, as the :class:`TestCaseResult`\s are the leaves
> of the hierarchy tree.
> >> +
> >> + Args:
> >> + statistics: The :class:`Statistics` object where the stats
> will be added.
> >> + """
> >> statistics += self.result
> >>
> >> def __bool__(self) -> bool:
> >> + """The test case passed only if setup, teardown and the test
> case itself passed."""
> >> return (
> >> bool(self.setup_result) and bool(self.teardown_result) and
> bool(self.result)
> >> )
> >>
> >>
> >> class TestSuiteResult(BaseResult):
> >> - """
> >> - The test suite specific result.
> >> - The _inner_results list stores results of test cases in a given
> test suite.
> >> - Also stores the test suite name.
> >> + """The test suite specific result.
> >> +
> >> + The internal list stores the results of all test cases in a given
> test suite.
> >> +
> >> + Attributes:
> >> + suite_name: The test suite name.
> >> """
> >>
> >
> > I think this should also be a class variable.
> >
> >
> >>
> >> suite_name: str
> >>
> >> def __init__(self, suite_name: str):
> >> + """Extend the constructor with `suite_name`.
> >> +
> >> + Args:
> >> + suite_name: The test suite's name.
> >> + """
> >> super(TestSuiteResult, self).__init__()
> >> self.suite_name = suite_name
> >>
> >> def add_test_case(self, test_case_name: str) -> TestCaseResult:
> >> + """Add and return the inner result (test case).
> >> +
> >> + Returns:
> >> + The test case's result.
> >> + """
> >> test_case_result = TestCaseResult(test_case_name)
> >> self._inner_results.append(test_case_result)
> >> return test_case_result
> >>
> >>
> >> class BuildTargetResult(BaseResult):
> >> - """
> >> - The build target specific result.
> >> - The _inner_results list stores results of test suites in a given
> build target.
> >> - Also stores build target specifics, such as compiler used to build
> DPDK.
> >> + """The build target specific result.
> >> +
> >> + The internal list stores the results of all test suites in a given
> build target.
> >> +
> >> + Attributes:
> >> + arch: The DPDK build target architecture.
> >> + os: The DPDK build target operating system.
> >> + cpu: The DPDK build target CPU.
> >> + compiler: The DPDK build target compiler.
> >> + compiler_version: The DPDK build target compiler version.
> >> + dpdk_version: The built DPDK version.
> >> """
> >
> >
> > I think this should be broken into class variables as well.
> >
> >>
> >>
> >> arch: Architecture
> >> @@ -213,6 +331,11 @@ class BuildTargetResult(BaseResult):
> >> dpdk_version: str | None
> >>
> >> def __init__(self, build_target: BuildTargetConfiguration):
> >> + """Extend the constructor with the `build_target`'s build
> target config.
> >> +
> >> + Args:
> >> + build_target: The build target's test run configuration.
> >> + """
> >> super(BuildTargetResult, self).__init__()
> >> self.arch = build_target.arch
> >> self.os = build_target.os
> >> @@ -222,20 +345,35 @@ def __init__(self, build_target:
> BuildTargetConfiguration):
> >> self.dpdk_version = None
> >>
> >> def add_build_target_info(self, versions: BuildTargetInfo) -> None:
> >> + """Add information about the build target gathered at runtime.
> >> +
> >> + Args:
> >> + versions: The additional information.
> >> + """
> >> self.compiler_version = versions.compiler_version
> >> self.dpdk_version = versions.dpdk_version
> >>
> >> def add_test_suite(self, test_suite_name: str) -> TestSuiteResult:
> >> + """Add and return the inner result (test suite).
> >> +
> >> + Returns:
> >> + The test suite's result.
> >> + """
> >> test_suite_result = TestSuiteResult(test_suite_name)
> >> self._inner_results.append(test_suite_result)
> >> return test_suite_result
> >>
> >>
> >> class ExecutionResult(BaseResult):
> >> - """
> >> - The execution specific result.
> >> - The _inner_results list stores results of build targets in a given
> execution.
> >> - Also stores the SUT node configuration.
> >> + """The execution specific result.
> >> +
> >> + The internal list stores the results of all build targets in a
> given execution.
> >> +
> >> + Attributes:
> >> + sut_node: The SUT node used in the execution.
> >> + sut_os_name: The operating system of the SUT node.
> >> + sut_os_version: The operating system version of the SUT node.
> >> + sut_kernel_version: The operating system kernel version of the
> SUT node.
> >> """
> >>
> >
> > I think these should be class variables as well.
> >
> >>
> >> sut_node: NodeConfiguration
> >> @@ -244,36 +382,55 @@ class ExecutionResult(BaseResult):
> >> sut_kernel_version: str
> >>
> >> def __init__(self, sut_node: NodeConfiguration):
> >> + """Extend the constructor with the `sut_node`'s config.
> >> +
> >> + Args:
> >> + sut_node: The SUT node's test run configuration used in
> the execution.
> >> + """
> >> super(ExecutionResult, self).__init__()
> >> self.sut_node = sut_node
> >>
> >> def add_build_target(
> >> self, build_target: BuildTargetConfiguration
> >> ) -> BuildTargetResult:
> >> + """Add and return the inner result (build target).
> >> +
> >> + Args:
> >> + build_target: The build target's test run configuration.
> >> +
> >> + Returns:
> >> + The build target's result.
> >> + """
> >> build_target_result = BuildTargetResult(build_target)
> >> self._inner_results.append(build_target_result)
> >> return build_target_result
> >>
> >> def add_sut_info(self, sut_info: NodeInfo) -> None:
> >> + """Add SUT information gathered at runtime.
> >> +
> >> + Args:
> >> + sut_info: The additional SUT node information.
> >> + """
> >> self.sut_os_name = sut_info.os_name
> >> self.sut_os_version = sut_info.os_version
> >> self.sut_kernel_version = sut_info.kernel_version
> >>
> >>
> >> class DTSResult(BaseResult):
> >> - """
> >> - Stores environment information and test results from a DTS run,
> which are:
> >> - * Execution level information, such as SUT and TG hardware.
> >> - * Build target level information, such as compiler, target OS and
> cpu.
> >> - * Test suite results.
> >> - * All errors that are caught and recorded during DTS execution.
> >> + """Stores environment information and test results from a DTS run.
> >>
> >> - The information is stored in nested objects.
> >> + * Execution level information, such as testbed and the test
> suite list,
> >> + * Build target level information, such as compiler, target OS
> and cpu,
> >> + * Test suite and test case results,
> >> + * All errors that are caught and recorded during DTS execution.
> >>
> >> - The class is capable of computing the return code used to exit DTS
> with
> >> - from the stored error.
> >> + The information is stored hierarchically. This is the first level
> of the hierarchy
> >> + and as such is where the data form the whole hierarchy is collated
> or processed.
> >>
> >> - It also provides a brief statistical summary of passed/failed test
> cases.
> >> + The internal list stores the results of all executions.
> >> +
> >> + Attributes:
> >> + dpdk_version: The DPDK version to record.
> >> """
> >>
> >
> > I think this should be a class variable as well.
> >
>
> This is the only place where making this a class variable would work,
> but I don't see a reason for it. An instance variable works just as
> well.
>
> >>
> >> dpdk_version: str | None
> >> @@ -284,6 +441,11 @@ class DTSResult(BaseResult):
> >> _stats_filename: str
> >>
> >> def __init__(self, logger: DTSLOG):
> >> + """Extend the constructor with top-level specifics.
> >> +
> >> + Args:
> >> + logger: The logger instance the whole result will use.
> >> + """
> >> super(DTSResult, self).__init__()
> >> self.dpdk_version = None
> >> self._logger = logger
> >> @@ -293,21 +455,33 @@ def __init__(self, logger: DTSLOG):
> >> self._stats_filename = os.path.join(SETTINGS.output_dir,
> "statistics.txt")
> >>
> >> def add_execution(self, sut_node: NodeConfiguration) ->
> ExecutionResult:
> >> + """Add and return the inner result (execution).
> >> +
> >> + Args:
> >> + sut_node: The SUT node's test run configuration.
> >> +
> >> + Returns:
> >> + The execution's result.
> >> + """
> >> execution_result = ExecutionResult(sut_node)
> >> self._inner_results.append(execution_result)
> >> return execution_result
> >>
> >> def add_error(self, error: Exception) -> None:
> >> + """Record an error that occurred outside any execution.
> >> +
> >> + Args:
> >> + error: The exception to record.
> >> + """
> >> self._errors.append(error)
> >>
> >> def process(self) -> None:
> >> - """
> >> - Process the data after a DTS run.
> >> - The data is added to nested objects during runtime and this
> parent object
> >> - is not updated at that time. This requires us to process the
> nested data
> >> - after it's all been gathered.
> >> + """Process the data after a whole DTS run.
> >> +
> >> + The data is added to inner objects during runtime and this
> object is not updated
> >> + at that time. This requires us to process the inner data after
> it's all been gathered.
> >>
> >> - The processing gathers all errors and the result statistics of
> test cases.
> >> + The processing gathers all errors and the statistics of test
> case results.
> >> """
> >> self._errors += self.get_errors()
> >> if self._errors and self._logger:
> >> @@ -321,8 +495,10 @@ def process(self) -> None:
> >> stats_file.write(str(self._stats_result))
> >>
> >> def get_return_code(self) -> int:
> >> - """
> >> - Go through all stored Exceptions and return the highest error
> code found.
> >> + """Go through all stored Exceptions and return the final DTS
> error code.
> >> +
> >> + Returns:
> >> + The highest error code found.
> >> """
> >> for error in self._errors:
> >> error_return_code = ErrorSeverity.GENERIC_ERR
> >> --
> >> 2.34.1
> >>
>
@@ -2,8 +2,25 @@
# Copyright(c) 2023 PANTHEON.tech s.r.o.
# Copyright(c) 2023 University of New Hampshire
-"""
-Generic result container and reporters
+r"""Record and process DTS results.
+
+The results are recorded in a hierarchical manner:
+
+ * :class:`DTSResult` contains
+ * :class:`ExecutionResult` contains
+ * :class:`BuildTargetResult` contains
+ * :class:`TestSuiteResult` contains
+ * :class:`TestCaseResult`
+
+Each result may contain multiple lower level results, e.g. there are multiple
+:class:`TestSuiteResult`\s in a :class:`BuildTargetResult`.
+The results have common parts, such as setup and teardown results, captured in :class:`BaseResult`,
+which also defines some common behaviors in its methods.
+
+Each result class has its own idiosyncrasies which they implement in overridden methods.
+
+The :option:`--output` command line argument and the :envvar:`DTS_OUTPUT_DIR` environment
+variable modify the directory where the files with results will be stored.
"""
import os.path
@@ -26,26 +43,34 @@
class Result(Enum):
- """
- An Enum defining the possible states that
- a setup, a teardown or a test case may end up in.
- """
+ """The possible states that a setup, a teardown or a test case may end up in."""
+ #:
PASS = auto()
+ #:
FAIL = auto()
+ #:
ERROR = auto()
+ #:
SKIP = auto()
def __bool__(self) -> bool:
+ """Only PASS is True."""
return self is self.PASS
class FixtureResult(object):
- """
- A record that stored the result of a setup or a teardown.
- The default is FAIL because immediately after creating the object
- the setup of the corresponding stage will be executed, which also guarantees
- the execution of teardown.
+ """A record that stores the result of a setup or a teardown.
+
+ FAIL is a sensible default since it prevents false positives
+ (which could happen if the default was PASS).
+
+ Preventing false positives or other false results is preferable since a failure
+ is mostly likely to be investigated (the other false results may not be investigated at all).
+
+ Attributes:
+ result: The associated result.
+ error: The error in case of a failure.
"""
result: Result
@@ -56,21 +81,32 @@ def __init__(
result: Result = Result.FAIL,
error: Exception | None = None,
):
+ """Initialize the constructor with the fixture result and store a possible error.
+
+ Args:
+ result: The result to store.
+ error: The error which happened when a failure occurred.
+ """
self.result = result
self.error = error
def __bool__(self) -> bool:
+ """A wrapper around the stored :class:`Result`."""
return bool(self.result)
class Statistics(dict):
- """
- A helper class used to store the number of test cases by its result
- along a few other basic information.
- Using a dict provides a convenient way to format the data.
+ """How many test cases ended in which result state along some other basic information.
+
+ Subclassing :class:`dict` provides a convenient way to format the data.
"""
def __init__(self, dpdk_version: str | None):
+ """Extend the constructor with relevant keys.
+
+ Args:
+ dpdk_version: The version of tested DPDK.
+ """
super(Statistics, self).__init__()
for result in Result:
self[result.name] = 0
@@ -78,8 +114,17 @@ def __init__(self, dpdk_version: str | None):
self["DPDK VERSION"] = dpdk_version
def __iadd__(self, other: Result) -> "Statistics":
- """
- Add a Result to the final count.
+ """Add a Result to the final count.
+
+ Example:
+ stats: Statistics = Statistics() # empty Statistics
+ stats += Result.PASS # add a Result to `stats`
+
+ Args:
+ other: The Result to add to this statistics object.
+
+ Returns:
+ The modified statistics object.
"""
self[other.name] += 1
self["PASS RATE"] = (
@@ -90,9 +135,7 @@ def __iadd__(self, other: Result) -> "Statistics":
return self
def __str__(self) -> str:
- """
- Provide a string representation of the data.
- """
+ """Each line contains the formatted key = value pair."""
stats_str = ""
for key, value in self.items():
stats_str += f"{key:<12} = {value}\n"
@@ -102,10 +145,16 @@ def __str__(self) -> str:
class BaseResult(object):
- """
- The Base class for all results. Stores the results of
- the setup and teardown portions of the corresponding stage
- and a list of results from each inner stage in _inner_results.
+ """Common data and behavior of DTS results.
+
+ Stores the results of the setup and teardown portions of the corresponding stage.
+ The hierarchical nature of DTS results is captured recursively in an internal list.
+ A stage is each level in this particular hierarchy (pre-execution or the top-most level,
+ execution, build target, test suite and test case.)
+
+ Attributes:
+ setup_result: The result of the setup of the particular stage.
+ teardown_result: The results of the teardown of the particular stage.
"""
setup_result: FixtureResult
@@ -113,15 +162,28 @@ class BaseResult(object):
_inner_results: MutableSequence["BaseResult"]
def __init__(self):
+ """Initialize the constructor."""
self.setup_result = FixtureResult()
self.teardown_result = FixtureResult()
self._inner_results = []
def update_setup(self, result: Result, error: Exception | None = None) -> None:
+ """Store the setup result.
+
+ Args:
+ result: The result of the setup.
+ error: The error that occurred in case of a failure.
+ """
self.setup_result.result = result
self.setup_result.error = error
def update_teardown(self, result: Result, error: Exception | None = None) -> None:
+ """Store the teardown result.
+
+ Args:
+ result: The result of the teardown.
+ error: The error that occurred in case of a failure.
+ """
self.teardown_result.result = result
self.teardown_result.error = error
@@ -141,27 +203,55 @@ def _get_inner_errors(self) -> list[Exception]:
]
def get_errors(self) -> list[Exception]:
+ """Compile errors from the whole result hierarchy.
+
+ Returns:
+ The errors from setup, teardown and all errors found in the whole result hierarchy.
+ """
return self._get_setup_teardown_errors() + self._get_inner_errors()
def add_stats(self, statistics: Statistics) -> None:
+ """Collate stats from the whole result hierarchy.
+
+ Args:
+ statistics: The :class:`Statistics` object where the stats will be collated.
+ """
for inner_result in self._inner_results:
inner_result.add_stats(statistics)
class TestCaseResult(BaseResult, FixtureResult):
- """
- The test case specific result.
- Stores the result of the actual test case.
- Also stores the test case name.
+ r"""The test case specific result.
+
+ Stores the result of the actual test case. This is done by adding an extra superclass
+ in :class:`FixtureResult`. The setup and teardown results are :class:`FixtureResult`\s and
+ the class is itself a record of the test case.
+
+ Attributes:
+ test_case_name: The test case name.
"""
test_case_name: str
def __init__(self, test_case_name: str):
+ """Extend the constructor with `test_case_name`.
+
+ Args:
+ test_case_name: The test case's name.
+ """
super(TestCaseResult, self).__init__()
self.test_case_name = test_case_name
def update(self, result: Result, error: Exception | None = None) -> None:
+ """Update the test case result.
+
+ This updates the result of the test case itself and doesn't affect
+ the results of the setup and teardown steps in any way.
+
+ Args:
+ result: The result of the test case.
+ error: The error that occurred in case of a failure.
+ """
self.result = result
self.error = error
@@ -171,38 +261,66 @@ def _get_inner_errors(self) -> list[Exception]:
return []
def add_stats(self, statistics: Statistics) -> None:
+ r"""Add the test case result to statistics.
+
+ The base method goes through the hierarchy recursively and this method is here to stop
+ the recursion, as the :class:`TestCaseResult`\s are the leaves of the hierarchy tree.
+
+ Args:
+ statistics: The :class:`Statistics` object where the stats will be added.
+ """
statistics += self.result
def __bool__(self) -> bool:
+ """The test case passed only if setup, teardown and the test case itself passed."""
return (
bool(self.setup_result) and bool(self.teardown_result) and bool(self.result)
)
class TestSuiteResult(BaseResult):
- """
- The test suite specific result.
- The _inner_results list stores results of test cases in a given test suite.
- Also stores the test suite name.
+ """The test suite specific result.
+
+ The internal list stores the results of all test cases in a given test suite.
+
+ Attributes:
+ suite_name: The test suite name.
"""
suite_name: str
def __init__(self, suite_name: str):
+ """Extend the constructor with `suite_name`.
+
+ Args:
+ suite_name: The test suite's name.
+ """
super(TestSuiteResult, self).__init__()
self.suite_name = suite_name
def add_test_case(self, test_case_name: str) -> TestCaseResult:
+ """Add and return the inner result (test case).
+
+ Returns:
+ The test case's result.
+ """
test_case_result = TestCaseResult(test_case_name)
self._inner_results.append(test_case_result)
return test_case_result
class BuildTargetResult(BaseResult):
- """
- The build target specific result.
- The _inner_results list stores results of test suites in a given build target.
- Also stores build target specifics, such as compiler used to build DPDK.
+ """The build target specific result.
+
+ The internal list stores the results of all test suites in a given build target.
+
+ Attributes:
+ arch: The DPDK build target architecture.
+ os: The DPDK build target operating system.
+ cpu: The DPDK build target CPU.
+ compiler: The DPDK build target compiler.
+ compiler_version: The DPDK build target compiler version.
+ dpdk_version: The built DPDK version.
"""
arch: Architecture
@@ -213,6 +331,11 @@ class BuildTargetResult(BaseResult):
dpdk_version: str | None
def __init__(self, build_target: BuildTargetConfiguration):
+ """Extend the constructor with the `build_target`'s build target config.
+
+ Args:
+ build_target: The build target's test run configuration.
+ """
super(BuildTargetResult, self).__init__()
self.arch = build_target.arch
self.os = build_target.os
@@ -222,20 +345,35 @@ def __init__(self, build_target: BuildTargetConfiguration):
self.dpdk_version = None
def add_build_target_info(self, versions: BuildTargetInfo) -> None:
+ """Add information about the build target gathered at runtime.
+
+ Args:
+ versions: The additional information.
+ """
self.compiler_version = versions.compiler_version
self.dpdk_version = versions.dpdk_version
def add_test_suite(self, test_suite_name: str) -> TestSuiteResult:
+ """Add and return the inner result (test suite).
+
+ Returns:
+ The test suite's result.
+ """
test_suite_result = TestSuiteResult(test_suite_name)
self._inner_results.append(test_suite_result)
return test_suite_result
class ExecutionResult(BaseResult):
- """
- The execution specific result.
- The _inner_results list stores results of build targets in a given execution.
- Also stores the SUT node configuration.
+ """The execution specific result.
+
+ The internal list stores the results of all build targets in a given execution.
+
+ Attributes:
+ sut_node: The SUT node used in the execution.
+ sut_os_name: The operating system of the SUT node.
+ sut_os_version: The operating system version of the SUT node.
+ sut_kernel_version: The operating system kernel version of the SUT node.
"""
sut_node: NodeConfiguration
@@ -244,36 +382,55 @@ class ExecutionResult(BaseResult):
sut_kernel_version: str
def __init__(self, sut_node: NodeConfiguration):
+ """Extend the constructor with the `sut_node`'s config.
+
+ Args:
+ sut_node: The SUT node's test run configuration used in the execution.
+ """
super(ExecutionResult, self).__init__()
self.sut_node = sut_node
def add_build_target(
self, build_target: BuildTargetConfiguration
) -> BuildTargetResult:
+ """Add and return the inner result (build target).
+
+ Args:
+ build_target: The build target's test run configuration.
+
+ Returns:
+ The build target's result.
+ """
build_target_result = BuildTargetResult(build_target)
self._inner_results.append(build_target_result)
return build_target_result
def add_sut_info(self, sut_info: NodeInfo) -> None:
+ """Add SUT information gathered at runtime.
+
+ Args:
+ sut_info: The additional SUT node information.
+ """
self.sut_os_name = sut_info.os_name
self.sut_os_version = sut_info.os_version
self.sut_kernel_version = sut_info.kernel_version
class DTSResult(BaseResult):
- """
- Stores environment information and test results from a DTS run, which are:
- * Execution level information, such as SUT and TG hardware.
- * Build target level information, such as compiler, target OS and cpu.
- * Test suite results.
- * All errors that are caught and recorded during DTS execution.
+ """Stores environment information and test results from a DTS run.
- The information is stored in nested objects.
+ * Execution level information, such as testbed and the test suite list,
+ * Build target level information, such as compiler, target OS and cpu,
+ * Test suite and test case results,
+ * All errors that are caught and recorded during DTS execution.
- The class is capable of computing the return code used to exit DTS with
- from the stored error.
+ The information is stored hierarchically. This is the first level of the hierarchy
+ and as such is where the data form the whole hierarchy is collated or processed.
- It also provides a brief statistical summary of passed/failed test cases.
+ The internal list stores the results of all executions.
+
+ Attributes:
+ dpdk_version: The DPDK version to record.
"""
dpdk_version: str | None
@@ -284,6 +441,11 @@ class DTSResult(BaseResult):
_stats_filename: str
def __init__(self, logger: DTSLOG):
+ """Extend the constructor with top-level specifics.
+
+ Args:
+ logger: The logger instance the whole result will use.
+ """
super(DTSResult, self).__init__()
self.dpdk_version = None
self._logger = logger
@@ -293,21 +455,33 @@ def __init__(self, logger: DTSLOG):
self._stats_filename = os.path.join(SETTINGS.output_dir, "statistics.txt")
def add_execution(self, sut_node: NodeConfiguration) -> ExecutionResult:
+ """Add and return the inner result (execution).
+
+ Args:
+ sut_node: The SUT node's test run configuration.
+
+ Returns:
+ The execution's result.
+ """
execution_result = ExecutionResult(sut_node)
self._inner_results.append(execution_result)
return execution_result
def add_error(self, error: Exception) -> None:
+ """Record an error that occurred outside any execution.
+
+ Args:
+ error: The exception to record.
+ """
self._errors.append(error)
def process(self) -> None:
- """
- Process the data after a DTS run.
- The data is added to nested objects during runtime and this parent object
- is not updated at that time. This requires us to process the nested data
- after it's all been gathered.
+ """Process the data after a whole DTS run.
+
+ The data is added to inner objects during runtime and this object is not updated
+ at that time. This requires us to process the inner data after it's all been gathered.
- The processing gathers all errors and the result statistics of test cases.
+ The processing gathers all errors and the statistics of test case results.
"""
self._errors += self.get_errors()
if self._errors and self._logger:
@@ -321,8 +495,10 @@ def process(self) -> None:
stats_file.write(str(self._stats_result))
def get_return_code(self) -> int:
- """
- Go through all stored Exceptions and return the highest error code found.
+ """Go through all stored Exceptions and return the final DTS error code.
+
+ Returns:
+ The highest error code found.
"""
for error in self._errors:
error_return_code = ErrorSeverity.GENERIC_ERR