<div dir="ltr"><div><div style="font-family:arial,sans-serif" class="gmail_default">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. </div></div><div> </div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Nov 15, 2023 at 8:12 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote: </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">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. """ </blockquote><div> </div><div><div style="font-family:arial,sans-serif" class="gmail_default">I think the items in the attributes section should instead be "#:" because they are class variables. </div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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. + """ </blockquote><div> </div><div><div style="font-family:arial,sans-serif" class="gmail_default">Should we maybe mark the "PASS RATE" and the "DPDK VERSION" as instance variables of the class? </div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> super(Statistics, self).__init__() for result in Result: self[<a href="http://result.name" rel="noreferrer" target="_blank">result.name</a>] = 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[<a href="http://other.name" rel="noreferrer" target="_blank">other.name</a>] += 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. """ </blockquote><div> </div><div><div style="font-family:arial,sans-serif" class="gmail_default">I think this might be another case of the attributes should be marked as class variables instead of instance variables. </div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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. """ </blockquote><div> </div><div><div style="font-family:arial,sans-serif" class="gmail_default">Another spot where I think this should have a class variable comment. </div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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. """ </blockquote><div> </div><div><div style="font-family:arial,sans-serif" class="gmail_default">I think this should also be a class variable. </div> </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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. """ </blockquote><div> </div><div><div style="font-family:arial,sans-serif" class="gmail_default">I think this should be broken into class variables as well. </div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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. """ </blockquote><div> </div><div><div style="font-family:arial,sans-serif" class="gmail_default">I think these should be class variables as well. </div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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. """ </blockquote><div> </div><div><div style="font-family:arial,sans-serif" class="gmail_default">I think this should be a class variable as well. </div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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 </blockquote></div></div>