<div dir="ltr"><div dir="ltr"><div class="gmail_default" style="font-family:arial,sans-serif"> </div></div> <div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Nov 20, 2023 at 11:33 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">On Thu, Nov 16, 2023 at 11:47 PM Jeremy Spewock <<a href="mailto:jspewock@iol.unh.edu" target="_blank">jspewock@iol.unh.edu</a>> 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? </blockquote><div> </div><div><div class="gmail_default" style="font-family:arial,sans-serif">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.</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. >> + """ > > > 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[<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. >> """ > > > 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 >> </blockquote></div></div>