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 B38DD44180; Fri, 7 Jun 2024 15:13:46 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 2806840151; Fri, 7 Jun 2024 15:13:46 +0200 (CEST) Received: from mail-lj1-f175.google.com (mail-lj1-f175.google.com [209.85.208.175]) by mails.dpdk.org (Postfix) with ESMTP id 546B940150 for ; Fri, 7 Jun 2024 15:13:43 +0200 (CEST) Received: by mail-lj1-f175.google.com with SMTP id 38308e7fff4ca-2eaa794eb9fso24940921fa.2 for ; Fri, 07 Jun 2024 06:13:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1717766023; x=1718370823; darn=dpdk.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=UP/P7AZqNkbrUSFem+OM2qJf916tVGw3/999qfcneGE=; b=olNq6D3wQQmOcPAD2i7jRPwChk/NBp+sVDaAzNLCdDRqMx7UFShQ7MuiDome+cwtS/ UsvTdsS+xmTB+RFlpswiM9QDuFgSWKA7dJ6rhHRX+TUad+LSdK6lFghc0NZxM0wop0sm vk2vNdAIOKhgXA3PfAI9sw2YJBwn5hMf6AvfSlqOBa3v+DeQlUJhDINE6DBrYFtFHpXI XUjEQhLXK88bFMsswOSmvsSWoHEy4JydWF79mJRV5IyEGJP41r/SKm9xcRXLJaydLEH7 uUVb1PVS72UiatZXE0kyzbJoQBcRgcaaUPGg/sBa+WOH6BXZq0g85LFZ//jDcNXpZTBx qZvw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1717766023; x=1718370823; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=UP/P7AZqNkbrUSFem+OM2qJf916tVGw3/999qfcneGE=; b=KK1XTllLapiLcNJBSA1nAxjXBXiDOVwXp8eAQlJNUf0uS5uj4Wwaj+oKeGzWEwVL1C L57LP2Xjwwu1fxpoChu56ruY6v4wQ3PwX2flqqtwyzurUJHj4p/JpJif0gJ8FmBlNIrA sx6+0H7ubTTAtD44EbKBMHEQbFWv26iHMCXA8jgjzD9nJDfxBhPUnuIpmFxN25W7qZDc JrsuOPYxjmxt38rbbp2CHYemvapceRL0o58XpHkcvIVjn+g/p/lxfNNHtQNVlGCHMfhW 8SmjyGse8fLjg7K3A9/BcnMhb3AeCVVvcFuIj/u19cbpD+M6LeSR1wtEoLE1lUlOt/Q0 or4g== X-Gm-Message-State: AOJu0Yzc2hWMqbRSWw7HbVxEszaDFE2xHiL0+2PFt/S+dvZu5mRpkiOQ b6gN7eFD98VUfo6QlwR9ssTAB1chDmha6RCAIIu5X0Lq3meq2GEM9nITxjSjqTw= X-Google-Smtp-Source: AGHT+IHD5SSJgodNrdNnF/B37XFy35r7zI4TU6M8hRyvYxNhNC3msWjezLtsUb/+cvSdxZPPK6C1uw== X-Received: by 2002:a2e:a16e:0:b0:2e9:79a4:3d1a with SMTP id 38308e7fff4ca-2eadce20acemr16768151fa.8.1717766022760; Fri, 07 Jun 2024 06:13:42 -0700 (PDT) Received: from [192.168.1.113] ([84.245.121.236]) by smtp.gmail.com with ESMTPSA id 4fb4d7f45d1cf-57aae0ca5e3sm2728694a12.25.2024.06.07.06.13.41 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 07 Jun 2024 06:13:41 -0700 (PDT) Message-ID: <044253c5-eeb4-47c4-ba66-36517d3b4add@pantheon.tech> Date: Fri, 7 Jun 2024 15:13:40 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [RFC PATCH v2] dts: skip test cases based on capabilities To: Luca Vizzarro , thomas@monjalon.net, Honnappa.Nagarahalli@arm.com, jspewock@iol.unh.edu, probb@iol.unh.edu, paul.szczepanek@arm.com, npratte@iol.unh.edu Cc: dev@dpdk.org References: <20240301155416.96960-1-juraj.linkes@pantheon.tech> <20240411084829.64984-1-juraj.linkes@pantheon.tech> Content-Language: en-US From: =?UTF-8?Q?Juraj_Linke=C5=A1?= In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 22. 5. 2024 16:58, Luca Vizzarro wrote: > Hi Juraj, > > Here's my review. Excuse me for the unordinary format, but I thought > it would have just been easier to convey my suggestions through code. > Apart from the smaller suggestions, the most important one I think is > that we should make sure to enforce type checking (and hinting). > Overall I like your approach, but I think it'd be better to initialise > all the required variables per test case, so we can access them > directly without doing checks everytime. The easiest approach I can see > to do this though, is to decorate all the test cases, for example > through @test. It'd be a somewhat important change as it changes the > test writing API, but it brings some improvements while making the > system more resilient. > The format is a bit wonky, but I was able to figure it out. I answered some of the questions I found. I like the idea of decorating the test cases. I was thinking of something similar in the past and now it makes sense to implement it. I'll definitely use some (or all :-)) of the suggestions. > The comments in the code are part of the review and may refer to > either your code or mine. The diff is in working order, so you could > test the functionality if you wished. > > Best regards, > Luca > > --- > diff --git a/dts/framework/remote_session/__init__.py > b/dts/framework/remote_session/__init__.py > index f18a9f2259..d4dfed3a58 100644 > --- a/dts/framework/remote_session/__init__.py > +++ b/dts/framework/remote_session/__init__.py > @@ -22,6 +22,9 @@ >  from .python_shell import PythonShell >  from .remote_session import CommandResult, RemoteSession >  from .ssh_session import SSHSession > + > +# in my testpmd params series these imports are removed as they promote > eager module loading, > +# significantly increasing the chances of circular dependencies >  from .testpmd_shell import NicCapability, TestPmdShell > > > diff --git a/dts/framework/remote_session/testpmd_shell.py > b/dts/framework/remote_session/testpmd_shell.py > index f6783af621..2b87e2e5c8 100644 > --- a/dts/framework/remote_session/testpmd_shell.py > +++ b/dts/framework/remote_session/testpmd_shell.py > @@ -16,7 +16,6 @@ >  """ > >  import time > -from collections.abc import MutableSet >  from enum import Enum, auto >  from functools import partial >  from pathlib import PurePath > @@ -232,9 +231,8 @@ def close(self) -> None: >          self.send_command("quit", "") >          return super().close() > > -    def get_capas_rxq( > -        self, supported_capabilities: MutableSet, > unsupported_capabilities: MutableSet > -    ) -> None: > +    # the built-in `set` is a mutable set. Is there an advantage to > using MutableSet? From what I can tell, it's best practice to be as broad as possible with input types. set is just one class, MutableSet could be any class that's a mutable set. > +    def get_capas_rxq(self, supported_capabilities: set, > unsupported_capabilities: set) -> None: >          """Get all rxq capabilities and divide them into supported and > unsupported. > >          Args: > @@ -243,6 +241,7 @@ def get_capas_rxq( >                  not supported will be stored. >          """ >          self._logger.debug("Getting rxq capabilities.") > +        # this assumes that the used ports are all the same. Could this > be of concern? Our current assumption is one NIC per one execution, so this should be safe, at least for now. >          command = "show rxq info 0 0" >          rxq_info = self.send_command(command) >          for line in rxq_info.split("\n"): > @@ -270,4 +269,6 @@ class NicCapability(Enum): >      `unsupported_capabilities` based on their support. >      """ > > +    # partial is just a high-order function that pre-fills the > arguments... but we have no arguments > +    # here? Was this intentional? It's necessary because of the interaction between Enums and functions. Without partial, accessing NicCapability.scattered_rx returns the function instead of the enum. >      scattered_rx = partial(TestPmdShell.get_capas_rxq)