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 760D943380; Mon, 20 Nov 2023 17:33:51 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 41C9642DDE; Mon, 20 Nov 2023 17:33:51 +0100 (CET) Received: from mail-ed1-f45.google.com (mail-ed1-f45.google.com [209.85.208.45]) by mails.dpdk.org (Postfix) with ESMTP id 9B58E42DD2 for ; Mon, 20 Nov 2023 17:33:50 +0100 (CET) Received: by mail-ed1-f45.google.com with SMTP id 4fb4d7f45d1cf-54366784377so6342265a12.3 for ; Mon, 20 Nov 2023 08:33:50 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1700498030; x=1701102830; darn=dpdk.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=5MvlackJJ8/4u1Ya1aWKDtNKZr3aL8q8GBgjClQutRQ=; b=NlNQfS4dg7sO7mA42hqiXYitOTmFzYtdGC8dbb6emb4vEm0CEvNDqgyjvk67b8N1Bn IE9gL1NDaICqGfhuMNJ9xG2H+HLXYXMP+wuh3keaxqprxntj2miG0tCPHavxyduz5o1p zoPyD8tfh2tfWnVwxVJwYzQWQYDenUX3P2m/MCrC0aM/0l8Cx6PwMu9TdNHrsQHhIDHd JJsDqqTas8CwsNN6/nTJBHhWBqzL06FzQgumbBgtipadbVU0uU2dkvRYa+owP+taQ/4D 6oC9FdDBtd7cav9Zwc4I6wyDOiQbD3q1+YAPi/S7h07zDOIQvsFTX6+QHuy1k6PulFuG Pwvg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1700498030; x=1701102830; h=content-transfer-encoding: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=5MvlackJJ8/4u1Ya1aWKDtNKZr3aL8q8GBgjClQutRQ=; b=Q0gPqbmoCYYT3tf8un6ZGxpV7nYq8Ul9XEUdNsvKrxBrQDAAcZFX+kKFlBC1sdBVvF UGne7qWzjdLb0gqRojQVxh+vfp9Zl70aUhBUSv9eZVmDXw1c9VHbLI/MdeuJGEN9lqn/ fcrXQYbGQX3JSD+s3Gra8gHlVuow6YLVZBVpZgq/aDvC2nDCDbJlncXFeA4Qe60hMP8i 3F+4AgnyruirZlTsbrSsnaZ3Tlb9W5IeGTlGe4oIntgQoV0QZ7IoXo52o6pCruZ3PXFZ KEK+XE5FQ+xrjuZGbMSyMHFC0eAz2Lzf7SQr2EQIVEDGdu6E1/tp3RiMiiH2oKeJI6ah EBaw== X-Gm-Message-State: AOJu0Yy64bBxtx+et7m0dpHOu68olv5cjZ35/7XPdXe3w3dClMnIqEKJ Yv+4uzQd22VfE/FHm9OBdKl3UTvOtGHabP4zIbEEEg== X-Google-Smtp-Source: AGHT+IEy1sHXag7KLseUZFxULxrB0gp4GSPUUDkAWs/SO0bdT+0gDsRCcYdnJ66fdVCQLveMnGAmoTVhjftac+4xPDs= X-Received: by 2002:a17:907:d50c:b0:9e2:b85e:59e2 with SMTP id wb12-20020a170907d50c00b009e2b85e59e2mr6285812ejc.32.1700498030077; Mon, 20 Nov 2023 08:33:50 -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: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Mon, 20 Nov 2023 17:33:39 +0100 Message-ID: Subject: Re: [PATCH v7 09/21] dts: test result docstring update To: Jeremy Spewock 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: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 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 pla= ces 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 mu= ltiple >> +:class:`TestSuiteResult`\s in a :class:`BuildTargetResult`. >> +The results have common parts, such as setup and teardown results, capt= ured in :class:`BaseResult`, >> +which also defines some common behaviors in its methods. >> + >> +Each result class has its own idiosyncrasies which they implement in ov= erridden methods. >> + >> +The :option:`--output` command line argument and the :envvar:`DTS_OUTPU= T_DIR` environment >> +variable modify the directory where the files with results will be stor= ed. >> """ >> >> 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 =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 g= uarantees >> - 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 sin= ce a failure >> + is mostly likely to be investigated (the other false results may no= t 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 "#:" becaus= e 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 =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 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 d= ata. >> """ >> >> 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 v= ariables 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 corres= ponding stage. >> + The hierarchical nature of DTS results is captured recursively in a= n internal list. >> + A stage is each level in this particular hierarchy (pre-execution o= r 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 =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_erro= rs() >> >> 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 a= n extra superclass >> + in :class:`FixtureResult`. The setup and teardown results are :clas= s:`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 aff= ect >> + 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 leaves = of the hierarchy tree. >> + >> + Args: >> + statistics: The :class:`Statistics` object where the stats = will be added. >> + """ >> statistics +=3D self.result >> >> def __bool__(self) -> bool: >> + """The test case passed only if setup, teardown and the test ca= se 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 tes= t 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 t= est 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 bu= ild 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 targe= t 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: BuildTargetConfig= uration): >> self.dpdk_version =3D 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 =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 given = execution. >> - Also stores the SUT node configuration. >> + """The execution specific result. >> + >> + The internal list stores the results of all build targets in a give= n 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 =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, whi= ch are: >> - * Execution level information, such as SUT and TG hardware. >> - * Build target level information, such as compiler, target OS and c= pu. >> - * 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 sui= te list, >> + * Build target level information, such as compiler, target OS a= nd 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 o= f 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 =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, "sta= tistics.txt") >> >> def add_execution(self, sut_node: NodeConfiguration) -> ExecutionRe= sult: >> + """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 par= ent object >> - is not updated at that time. This requires us to process the ne= sted 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 obje= ct 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 ca= se 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 c= ode found. >> + """Go through all stored Exceptions and return the final DTS er= ror code. >> + >> + Returns: >> + The highest error code found. >> """ >> for error in self._errors: >> error_return_code =3D ErrorSeverity.GENERIC_ERR >> -- >> 2.34.1 >>