From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id A38B843404; Thu, 30 Nov 2023 22:20:32 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 8FCCA40277; Thu, 30 Nov 2023 22:20:32 +0100 (CET) Received: from mail-pj1-f46.google.com (mail-pj1-f46.google.com [209.85.216.46]) by mails.dpdk.org (Postfix) with ESMTP id F022A40266 for ; Thu, 30 Nov 2023 22:20:30 +0100 (CET) Received: by mail-pj1-f46.google.com with SMTP id 98e67ed59e1d1-280351c32afso1404714a91.1 for ; Thu, 30 Nov 2023 13:20:30 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1701379230; x=1701984030; darn=dpdk.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=8KjGCOWG7Kv5V0LBDeJmVpC5UaeSbG6TaXc2x448mYY=; b=iNVMBjxdeDGkiMkNaMqiNcL14ePha0mdDA6uJ4Zpr13mcp2zsJz5eIvSColD400iFL F1FOLqggB4pRbObpsXuE/YG/llnDMIzRPOg57333U1ohDteD9MALJkqGPOU+ShHtPnRd wSTT2dtw25dkNEG/haeDplCBgAHpKmJbdRrnU= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1701379230; x=1701984030; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=8KjGCOWG7Kv5V0LBDeJmVpC5UaeSbG6TaXc2x448mYY=; b=cI7z4HRJv+4E2BEP1ehhgjCDEEvKgcV1MuLqOpVd1ylmAmxDDZSQtUm0FXvva9sOKE 7hIVYEhIv4lzC7rcOawd8htaKy1deU7HNNVHHCD6k2TKCrt/YSwR7hF4xRL/UuqWdYJX dxlVHHypSzl9qVeDCnEbMfQD2+HDP7nm+T5gJ3YHbGirmm1I775pocMdv59Mzu/z5f0j n0TWhE66ydMmptEijCQFi5I8Y/ddl/npHh0X5HNO15CYc9VOaRV9R8Q+1+Y6xycEyoF6 8Kfc/y3eT5AFC9Zk1euQbMsshuTRiJuO4KDo0ZwuxyrT4dXzHqfz2OpvBdvZ+jn+xmY3 8y6A== X-Gm-Message-State: AOJu0YxEcpjZlqMX6TvVjkZwjPf/ZTL0COpqK/ZBlGQToWf0+Tp8ADkt eFEa0bbgZKRd2bmvR1NQXqnPA4pj4D++WgxjN8ogSg== X-Google-Smtp-Source: AGHT+IFHSkrdzfT1gafCfZUn5LOd7tYQ2bU2ZNrszyOFno4t8CJA1TpBnqpkjdw1E0axBYXFCbfRM8aFiTld+dHUSVE= X-Received: by 2002:a17:90b:4b4b:b0:285:b784:75d9 with SMTP id mi11-20020a17090b4b4b00b00285b78475d9mr19450646pjb.12.1701379230037; Thu, 30 Nov 2023 13:20:30 -0800 (PST) MIME-Version: 1.0 References: <20231108125324.191005-23-juraj.linkes@pantheon.tech> <20231115130959.39420-1-juraj.linkes@pantheon.tech> <20231115130959.39420-10-juraj.linkes@pantheon.tech> In-Reply-To: From: Jeremy Spewock Date: Thu, 30 Nov 2023 16:20:19 -0500 Message-ID: Subject: Re: [PATCH v7 09/21] dts: test result docstring update To: =?UTF-8?Q?Juraj_Linke=C5=A1?= Cc: thomas@monjalon.net, Honnappa.Nagarahalli@arm.com, probb@iol.unh.edu, paul.szczepanek@arm.com, yoan.picchi@foss.arm.com, dev@dpdk.org Content-Type: multipart/alternative; boundary="000000000000a297a1060b6538dd" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org --000000000000a297a1060b6538dd Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, Nov 20, 2023 at 11:33=E2=80=AFAM Juraj Linke=C5=A1 wrote: > On Thu, Nov 16, 2023 at 11:47=E2=80=AFPM Jeremy Spewock > 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=E2=80=AFAM Juraj Linke=C5=A1 > wrote: > >> > >> Format according to the Google format and PEP257, with slight > >> deviations. > >> > >> Signed-off-by: Juraj Linke=C5=A1 > >> --- > >> 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 ma= y > end up in.""" > >> > >> + #: > >> PASS =3D auto() > >> + #: > >> FAIL =3D auto() > >> + #: > >> ERROR =3D auto() > >> + #: > >> SKIP =3D 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 =3D Result.FAIL, > >> error: Exception | None =3D 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 =3D result > >> self.error =3D 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 resu= lt > >> - 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] =3D 0 > >> @@ -78,8 +114,17 @@ def __init__(self, dpdk_version: str | None): > >> self["DPDK VERSION"] =3D 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 =3D Statistics() # empty Statistics > >> + stats +=3D 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] +=3D 1 > >> self["PASS RATE"] =3D ( > >> @@ -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 =3D value pair.""" > >> stats_str =3D "" > >> for key, value in self.items(): > >> stats_str +=3D f"{key:<12} =3D {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 particula= r > stage. > >> """ > > > > > > I think this might be another case of the attributes should be marked a= s > 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 =3D FixtureResult() > >> self.teardown_result =3D FixtureResult() > >> self._inner_results =3D [] > >> > >> def update_setup(self, result: Result, error: Exception | None = =3D > 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 =3D result > >> self.setup_result.error =3D error > >> > >> def update_teardown(self, result: Result, error: Exception | None > =3D 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 =3D result > >> self.teardown_result.error =3D 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 stat= s > 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 =3D test_case_name > >> > >> def update(self, result: Result, error: Exception | None =3D 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 =3D result > >> self.error =3D 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 leave= s > of the hierarchy tree. > >> + > >> + Args: > >> + statistics: The :class:`Statistics` object where the stat= s > will be added. > >> + """ > >> statistics +=3D 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) an= d > 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 =3D 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 =3D 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 buil= d > DPDK. > >> + """The build target specific result. > >> + > >> + The internal list stores the results of all test suites in a give= n > 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 =3D build_target.arch > >> self.os =3D build_target.os > >> @@ -222,20 +345,35 @@ def __init__(self, build_target: > BuildTargetConfiguration): > >> self.dpdk_version =3D None > >> > >> def add_build_target_info(self, versions: BuildTargetInfo) -> Non= e: > >> + """Add information about the build target gathered at runtime= . > >> + > >> + Args: > >> + versions: The additional information. > >> + """ > >> self.compiler_version =3D versions.compiler_version > >> self.dpdk_version =3D 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 =3D 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 give= n > 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 th= e > 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 =3D 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 =3D 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 =3D sut_info.os_name > >> self.sut_os_version =3D sut_info.os_version > >> self.sut_kernel_version =3D 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 executio= n. > >> > >> - The class is capable of computing the return code used to exit DT= S > 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 collate= d > or processed. > >> > >> - It also provides a brief statistical summary of passed/failed tes= t > 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 =3D None > >> self._logger =3D logger > >> @@ -293,21 +455,33 @@ def __init__(self, logger: DTSLOG): > >> self._stats_filename =3D 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 =3D 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 afte= r > it's all been gathered. > >> > >> - The processing gathers all errors and the result statistics o= f > test cases. > >> + The processing gathers all errors and the statistics of test > case results. > >> """ > >> self._errors +=3D 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 =3D ErrorSeverity.GENERIC_ERR > >> -- > >> 2.34.1 > >> > --000000000000a297a1060b6538dd Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


<= div dir=3D"ltr" class=3D"gmail_attr">On Mon, Nov 20, 2023 at 11:33=E2=80=AF= AM Juraj Linke=C5=A1 <juraj.linkes@pantheon.tech> wrote:
On Thu, Nov 16, 2023 at 11:4= 7=E2=80=AFPM Jeremy Spewock <jspewock@iol.unh.edu> wrote:
>
> The only comments I had on this were a few places where I think attrib= ute 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=E2=80=AFAM Juraj Linke=C5=A1 <juraj.li= nkes@pantheon.tech> wrote:
>>
>> Format according to the Google format and PEP257, with slight
>> deviations.
>>
>> Signed-off-by: Juraj Linke=C5=A1 <juraj.linkes@pantheon.tech>= ;
>> ---
>>=C2=A0 dts/framework/test_result.py | 292 +++++++++++++++++++++++++= +++-------
>>=C2=A0 1 file changed, 234 insertions(+), 58 deletions(-)
>>
>> diff --git a/dts/framework/test_result.py b/dts/framework/test_res= ult.py
>> index 603e18872c..05e210f6e7 100644
>> --- a/dts/framework/test_result.py
>> +++ b/dts/framework/test_result.py
>> @@ -2,8 +2,25 @@
>>=C2=A0 # Copyright(c) 2023 PANTHEON.tech s.r.o.
>>=C2=A0 # 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:
>> +
>> +=C2=A0 =C2=A0 * :class:`DTSResult` contains
>> +=C2=A0 =C2=A0 * :class:`ExecutionResult` contains
>> +=C2=A0 =C2=A0 * :class:`BuildTargetResult` contains
>> +=C2=A0 =C2=A0 * :class:`TestSuiteResult` contains
>> +=C2=A0 =C2=A0 * :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 b= e stored.
>>=C2=A0 """
>>
>>=C2=A0 import os.path
>> @@ -26,26 +43,34 @@
>>
>>
>>=C2=A0 class Result(Enum):
>> -=C2=A0 =C2=A0 """
>> -=C2=A0 =C2=A0 An Enum defining the possible states that
>> -=C2=A0 =C2=A0 a setup, a teardown or a test case may end up in. >> -=C2=A0 =C2=A0 """
>> +=C2=A0 =C2=A0 """The possible states that a setup,= a teardown or a test case may end up in."""
>>
>> +=C2=A0 =C2=A0 #:
>>=C2=A0 =C2=A0 =C2=A0 PASS =3D auto()
>> +=C2=A0 =C2=A0 #:
>>=C2=A0 =C2=A0 =C2=A0 FAIL =3D auto()
>> +=C2=A0 =C2=A0 #:
>>=C2=A0 =C2=A0 =C2=A0 ERROR =3D auto()
>> +=C2=A0 =C2=A0 #:
>>=C2=A0 =C2=A0 =C2=A0 SKIP =3D auto()
>>
>>=C2=A0 =C2=A0 =C2=A0 def __bool__(self) -> bool:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Only PASS is True.&= quot;""
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return self is self.PASS
>>
>>
>>=C2=A0 class FixtureResult(object):
>> -=C2=A0 =C2=A0 """
>> -=C2=A0 =C2=A0 A record that stored the result of a setup or a tea= rdown.
>> -=C2=A0 =C2=A0 The default is FAIL because immediately after creat= ing the object
>> -=C2=A0 =C2=A0 the setup of the corresponding stage will be execut= ed, which also guarantees
>> -=C2=A0 =C2=A0 the execution of teardown.
>> +=C2=A0 =C2=A0 """A record that stores the result o= f a setup or a teardown.
>> +
>> +=C2=A0 =C2=A0 FAIL is a sensible default since it prevents false = positives
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 (which could happen if the default wa= s PASS).
>> +
>> +=C2=A0 =C2=A0 Preventing false positives or other false results i= s preferable since a failure
>> +=C2=A0 =C2=A0 is mostly likely to be investigated (the other fals= e results may not be investigated at all).
>> +
>> +=C2=A0 =C2=A0 Attributes:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 result: The associated result.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 error: The error in case of a failure= .
>>=C2=A0 =C2=A0 =C2=A0 """
>
>
> 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 s= ense. I guess I was thinking of class variables as anything we statically d= efine 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. Us= ing the definition of instance variables as they can differ between instanc= es I agree makes this comment and the other ones you touched on obsolete.
=C2=A0
result.name] =3D= 0
>> @@ -78,8 +114,17 @@ def __init__(self, dpdk_version: str | None):<= br> >>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self["DPDK VERSION"] = =3D dpdk_version
>>
>>=C2=A0 =C2=A0 =C2=A0 def __iadd__(self, other: Result) -> "= Statistics":
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 Add a Result to the final count.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Add a Result to the= final count.
>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Example:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 stats: Statistics =3D S= tatistics()=C2=A0 # empty Statistics
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 stats +=3D Result.PASS= =C2=A0 # add a Result to `stats`
>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Args:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 other: The Result to ad= d to this statistics object.
>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Returns:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 The modified statistics= object.
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self[other.name] +=3D 1
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self["PASS RATE"] =3D = (
>> @@ -90,9 +135,7 @@ def __iadd__(self, other: Result) -> "S= tatistics":
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return self
>>
>>=C2=A0 =C2=A0 =C2=A0 def __str__(self) -> str:
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 Provide a string representation of th= e data.
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Each line contains = the formatted key =3D value pair."""
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 stats_str =3D ""
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 for key, value in self.items():<= br> >>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 stats_str +=3D f&q= uot;{key:<12} =3D {value}\n"
>> @@ -102,10 +145,16 @@ def __str__(self) -> str:
>>
>>
>>=C2=A0 class BaseResult(object):
>> -=C2=A0 =C2=A0 """
>> -=C2=A0 =C2=A0 The Base class for all results. Stores the results = of
>> -=C2=A0 =C2=A0 the setup and teardown portions of the correspondin= g stage
>> -=C2=A0 =C2=A0 and a list of results from each inner stage in _inn= er_results.
>> +=C2=A0 =C2=A0 """Common data and behavior of DTS r= esults.
>> +
>> +=C2=A0 =C2=A0 Stores the results of the setup and teardown portio= ns of the corresponding stage.
>> +=C2=A0 =C2=A0 The hierarchical nature of DTS results is captured = recursively in an internal list.
>> +=C2=A0 =C2=A0 A stage is each level in this particular hierarchy = (pre-execution or the top-most level,
>> +=C2=A0 =C2=A0 execution, build target, test suite and test case.)=
>> +
>> +=C2=A0 =C2=A0 Attributes:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 setup_result: The result of the setup= of the particular stage.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 teardown_result: The results of the t= eardown of the particular stage.
>>=C2=A0 =C2=A0 =C2=A0 """
>
>
> 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.

>>
>>
>>=C2=A0 =C2=A0 =C2=A0 setup_result: FixtureResult
>> @@ -113,15 +162,28 @@ class BaseResult(object):
>>=C2=A0 =C2=A0 =C2=A0 _inner_results: MutableSequence["BaseResu= lt"]
>>
>>=C2=A0 =C2=A0 =C2=A0 def __init__(self):
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Initialize the cons= tructor."""
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.setup_result =3D FixtureRes= ult()
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.teardown_result =3D Fixture= Result()
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._inner_results =3D []
>>
>>=C2=A0 =C2=A0 =C2=A0 def update_setup(self, result: Result, error: = Exception | None =3D None) -> None:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Store the setup res= ult.
>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Args:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 result: The result of t= he setup.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 error: The error that o= ccurred in case of a failure.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.setup_result.result =3D res= ult
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.setup_result.error =3D erro= r
>>
>>=C2=A0 =C2=A0 =C2=A0 def update_teardown(self, result: Result, erro= r: Exception | None =3D None) -> None:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Store the teardown = result.
>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Args:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 result: The result of t= he teardown.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 error: The error that o= ccurred in case of a failure.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.teardown_result.result =3D = result
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.teardown_result.error =3D e= rror
>>
>> @@ -141,27 +203,55 @@ def _get_inner_errors(self) -> list[Excep= tion]:
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ]
>>
>>=C2=A0 =C2=A0 =C2=A0 def get_errors(self) -> list[Exception]: >> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Compile errors from= the whole result hierarchy.
>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Returns:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 The errors from setup, = teardown and all errors found in the whole result hierarchy.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return self._get_setup_teardown_= errors() + self._get_inner_errors()
>>
>>=C2=A0 =C2=A0 =C2=A0 def add_stats(self, statistics: Statistics) -&= gt; None:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Collate stats from = the whole result hierarchy.
>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Args:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 statistics: The :class:= `Statistics` object where the stats will be collated.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 for inner_result in self._inner_= results:
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 inner_result.add_s= tats(statistics)
>>
>>
>>=C2=A0 class TestCaseResult(BaseResult, FixtureResult):
>> -=C2=A0 =C2=A0 """
>> -=C2=A0 =C2=A0 The test case specific result.
>> -=C2=A0 =C2=A0 Stores the result of the actual test case.
>> -=C2=A0 =C2=A0 Also stores the test case name.
>> +=C2=A0 =C2=A0 r"""The test case specific result. >> +
>> +=C2=A0 =C2=A0 Stores the result of the actual test case. This is = done by adding an extra superclass
>> +=C2=A0 =C2=A0 in :class:`FixtureResult`. The setup and teardown r= esults are :class:`FixtureResult`\s and
>> +=C2=A0 =C2=A0 the class is itself a record of the test case.
>> +
>> +=C2=A0 =C2=A0 Attributes:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 test_case_name: The test case name. >>=C2=A0 =C2=A0 =C2=A0 """
>>
>
> Another spot where I think this should have a class variable comment.<= br> >
>>
>>=C2=A0 =C2=A0 =C2=A0 test_case_name: str
>>
>>=C2=A0 =C2=A0 =C2=A0 def __init__(self, test_case_name: str):
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Extend the construc= tor with `test_case_name`.
>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Args:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 test_case_name: The tes= t case's name.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 super(TestCaseResult, self).__in= it__()
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.test_case_name =3D test_cas= e_name
>>
>>=C2=A0 =C2=A0 =C2=A0 def update(self, result: Result, error: Except= ion | None =3D None) -> None:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Update the test cas= e result.
>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 This updates the result of the test c= ase itself and doesn't affect
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 the results of the setup and teardown= steps in any way.
>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Args:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 result: The result of t= he test case.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 error: The error that o= ccurred in case of a failure.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.result =3D result
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.error =3D error
>>
>> @@ -171,38 +261,66 @@ def _get_inner_errors(self) -> list[Excep= tion]:
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return []
>>
>>=C2=A0 =C2=A0 =C2=A0 def add_stats(self, statistics: Statistics) -&= gt; None:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 r"""Add the test case = result to statistics.
>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 The base method goes through the hier= archy recursively and this method is here to stop
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 the recursion, as the :class:`TestCas= eResult`\s are the leaves of the hierarchy tree.
>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Args:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 statistics: The :class:= `Statistics` object where the stats will be added.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 statistics +=3D self.result
>>
>>=C2=A0 =C2=A0 =C2=A0 def __bool__(self) -> bool:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """The test case passe= d only if setup, teardown and the test case itself passed."""= ;
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return (
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 bool(self.setup_re= sult) and bool(self.teardown_result) and bool(self.result)
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 )
>>
>>
>>=C2=A0 class TestSuiteResult(BaseResult):
>> -=C2=A0 =C2=A0 """
>> -=C2=A0 =C2=A0 The test suite specific result.
>> -=C2=A0 =C2=A0 The _inner_results list stores results of test case= s in a given test suite.
>> -=C2=A0 =C2=A0 Also stores the test suite name.
>> +=C2=A0 =C2=A0 """The test suite specific result. >> +
>> +=C2=A0 =C2=A0 The internal list stores the results of all test ca= ses in a given test suite.
>> +
>> +=C2=A0 =C2=A0 Attributes:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 suite_name: The test suite name.
>>=C2=A0 =C2=A0 =C2=A0 """
>>
>
> I think this should also be a class variable.
>
>
>>
>>=C2=A0 =C2=A0 =C2=A0 suite_name: str
>>
>>=C2=A0 =C2=A0 =C2=A0 def __init__(self, suite_name: str):
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Extend the construc= tor with `suite_name`.
>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Args:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 suite_name: The test su= ite's name.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 super(TestSuiteResult, self).__i= nit__()
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.suite_name =3D suite_name >>
>>=C2=A0 =C2=A0 =C2=A0 def add_test_case(self, test_case_name: str) -= > TestCaseResult:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Add and return the = inner result (test case).
>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Returns:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 The test case's res= ult.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 test_case_result =3D TestCaseRes= ult(test_case_name)
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._inner_results.append(test_= case_result)
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return test_case_result
>>
>>
>>=C2=A0 class BuildTargetResult(BaseResult):
>> -=C2=A0 =C2=A0 """
>> -=C2=A0 =C2=A0 The build target specific result.
>> -=C2=A0 =C2=A0 The _inner_results list stores results of test suit= es in a given build target.
>> -=C2=A0 =C2=A0 Also stores build target specifics, such as compile= r used to build DPDK.
>> +=C2=A0 =C2=A0 """The build target specific result.=
>> +
>> +=C2=A0 =C2=A0 The internal list stores the results of all test su= ites in a given build target.
>> +
>> +=C2=A0 =C2=A0 Attributes:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 arch: The DPDK build target architect= ure.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 os: The DPDK build target operating s= ystem.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 cpu: The DPDK build target CPU.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 compiler: The DPDK build target compi= ler.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 compiler_version: The DPDK build targ= et compiler version.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 dpdk_version: The built DPDK version.=
>>=C2=A0 =C2=A0 =C2=A0 """
>
>
> I think this should be broken into class variables as well.
>
>>
>>
>>=C2=A0 =C2=A0 =C2=A0 arch: Architecture
>> @@ -213,6 +331,11 @@ class BuildTargetResult(BaseResult):
>>=C2=A0 =C2=A0 =C2=A0 dpdk_version: str | None
>>
>>=C2=A0 =C2=A0 =C2=A0 def __init__(self, build_target: BuildTargetCo= nfiguration):
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Extend the construc= tor with the `build_target`'s build target config.
>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Args:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 build_target: The build= target's test run configuration.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 super(BuildTargetResult, self)._= _init__()
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.arch =3D build_target.arch<= br> >>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.os =3D build_target.os
>> @@ -222,20 +345,35 @@ def __init__(self, build_target: BuildTarget= Configuration):
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.dpdk_version =3D None
>>
>>=C2=A0 =C2=A0 =C2=A0 def add_build_target_info(self, versions: Buil= dTargetInfo) -> None:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Add information abo= ut the build target gathered at runtime.
>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Args:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 versions: The additiona= l information.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.compiler_version =3D versio= ns.compiler_version
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.dpdk_version =3D versions.d= pdk_version
>>
>>=C2=A0 =C2=A0 =C2=A0 def add_test_suite(self, test_suite_name: str)= -> TestSuiteResult:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Add and return the = inner result (test suite).
>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Returns:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 The test suite's re= sult.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 test_suite_result =3D TestSuiteR= esult(test_suite_name)
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._inner_results.append(test_= suite_result)
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return test_suite_result
>>
>>
>>=C2=A0 class ExecutionResult(BaseResult):
>> -=C2=A0 =C2=A0 """
>> -=C2=A0 =C2=A0 The execution specific result.
>> -=C2=A0 =C2=A0 The _inner_results list stores results of build tar= gets in a given execution.
>> -=C2=A0 =C2=A0 Also stores the SUT node configuration.
>> +=C2=A0 =C2=A0 """The execution specific result. >> +
>> +=C2=A0 =C2=A0 The internal list stores the results of all build t= argets in a given execution.
>> +
>> +=C2=A0 =C2=A0 Attributes:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 sut_node: The SUT node used in the ex= ecution.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 sut_os_name: The operating system of = the SUT node.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 sut_os_version: The operating system = version of the SUT node.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 sut_kernel_version: The operating sys= tem kernel version of the SUT node.
>>=C2=A0 =C2=A0 =C2=A0 """
>>
>
> I think these should be class variables as well.
>
>>
>>=C2=A0 =C2=A0 =C2=A0 sut_node: NodeConfiguration
>> @@ -244,36 +382,55 @@ class ExecutionResult(BaseResult):
>>=C2=A0 =C2=A0 =C2=A0 sut_kernel_version: str
>>
>>=C2=A0 =C2=A0 =C2=A0 def __init__(self, sut_node: NodeConfiguration= ):
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Extend the construc= tor with the `sut_node`'s config.
>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Args:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 sut_node: The SUT node&= #39;s test run configuration used in the execution.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 super(ExecutionResult, self).__i= nit__()
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.sut_node =3D sut_node
>>
>>=C2=A0 =C2=A0 =C2=A0 def add_build_target(
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self, build_target: BuildTargetC= onfiguration
>>=C2=A0 =C2=A0 =C2=A0 ) -> BuildTargetResult:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Add and return the = inner result (build target).
>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Args:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 build_target: The build= target's test run configuration.
>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Returns:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 The build target's = result.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 build_target_result =3D BuildTar= getResult(build_target)
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._inner_results.append(build= _target_result)
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return build_target_result
>>
>>=C2=A0 =C2=A0 =C2=A0 def add_sut_info(self, sut_info: NodeInfo) -&g= t; None:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Add SUT information= gathered at runtime.
>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Args:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 sut_info: The additiona= l SUT node information.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.sut_os_name =3D sut_info.os= _name
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.sut_os_version =3D sut_info= .os_version
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.sut_kernel_version =3D sut_= info.kernel_version
>>
>>
>>=C2=A0 class DTSResult(BaseResult):
>> -=C2=A0 =C2=A0 """
>> -=C2=A0 =C2=A0 Stores environment information and test results fro= m a DTS run, which are:
>> -=C2=A0 =C2=A0 * Execution level information, such as SUT and TG h= ardware.
>> -=C2=A0 =C2=A0 * Build target level information, such as compiler,= target OS and cpu.
>> -=C2=A0 =C2=A0 * Test suite results.
>> -=C2=A0 =C2=A0 * All errors that are caught and recorded during DT= S execution.
>> +=C2=A0 =C2=A0 """Stores environment information an= d test results from a DTS run.
>>
>> -=C2=A0 =C2=A0 The information is stored in nested objects.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 * Execution level information, such a= s testbed and the test suite list,
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 * Build target level information, suc= h as compiler, target OS and cpu,
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 * Test suite and test case results, >> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 * All errors that are caught and reco= rded during DTS execution.
>>
>> -=C2=A0 =C2=A0 The class is capable of computing the return code u= sed to exit DTS with
>> -=C2=A0 =C2=A0 from the stored error.
>> +=C2=A0 =C2=A0 The information is stored hierarchically. This is t= he first level of the hierarchy
>> +=C2=A0 =C2=A0 and as such is where the data form the whole hierar= chy is collated or processed.
>>
>> -=C2=A0 =C2=A0 It also provides a brief statistical summary of pas= sed/failed test cases.
>> +=C2=A0 =C2=A0 The internal list stores the results of all executi= ons.
>> +
>> +=C2=A0 =C2=A0 Attributes:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 dpdk_version: The DPDK version to rec= ord.
>>=C2=A0 =C2=A0 =C2=A0 """
>>
>
> 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.

>>
>>=C2=A0 =C2=A0 =C2=A0 dpdk_version: str | None
>> @@ -284,6 +441,11 @@ class DTSResult(BaseResult):
>>=C2=A0 =C2=A0 =C2=A0 _stats_filename: str
>>
>>=C2=A0 =C2=A0 =C2=A0 def __init__(self, logger: DTSLOG):
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Extend the construc= tor with top-level specifics.
>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Args:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 logger: The logger inst= ance the whole result will use.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 super(DTSResult, self).__init__(= )
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.dpdk_version =3D None
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._logger =3D logger
>> @@ -293,21 +455,33 @@ def __init__(self, logger: DTSLOG):
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._stats_filename =3D os.path= .join(SETTINGS.output_dir, "statistics.txt")
>>
>>=C2=A0 =C2=A0 =C2=A0 def add_execution(self, sut_node: NodeConfigur= ation) -> ExecutionResult:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Add and return the = inner result (execution).
>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Args:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 sut_node: The SUT node&= #39;s test run configuration.
>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Returns:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 The execution's res= ult.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 execution_result =3D ExecutionRe= sult(sut_node)
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._inner_results.append(execu= tion_result)
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return execution_result
>>
>>=C2=A0 =C2=A0 =C2=A0 def add_error(self, error: Exception) -> No= ne:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Record an error tha= t occurred outside any execution.
>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Args:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 error: The exception to= record.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._errors.append(error)
>>
>>=C2=A0 =C2=A0 =C2=A0 def process(self) -> None:
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 Process the data after a DTS run.
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 The data is added to nested objects d= uring runtime and this parent object
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 is not updated at that time. This req= uires us to process the nested data
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 after it's all been gathered.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Process the data af= ter a whole DTS run.
>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 The data is added to inner objects du= ring runtime and this object is not updated
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 at that time. This requires us to pro= cess the inner data after it's all been gathered.
>>
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 The processing gathers all errors and= the result statistics of test cases.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 The processing gathers all errors and= the statistics of test case results.
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._errors +=3D self.get_error= s()
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if self._errors and self._logger= :
>> @@ -321,8 +495,10 @@ def process(self) -> None:
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 stats_file.write(s= tr(self._stats_result))
>>
>>=C2=A0 =C2=A0 =C2=A0 def get_return_code(self) -> int:
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 Go through all stored Exceptions and = return the highest error code found.
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Go through all stor= ed Exceptions and return the final DTS error code.
>> +
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Returns:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 The highest error code = found.
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 for error in self._errors:
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 error_return_code = =3D ErrorSeverity.GENERIC_ERR
>> --
>> 2.34.1
>>
--000000000000a297a1060b6538dd--