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 A319845955; Tue, 10 Sep 2024 13:05:33 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 7FECC402AB; Tue, 10 Sep 2024 13:05:33 +0200 (CEST) Received: from mail-lf1-f53.google.com (mail-lf1-f53.google.com [209.85.167.53]) by mails.dpdk.org (Postfix) with ESMTP id E4868400D6 for ; Tue, 10 Sep 2024 13:05:31 +0200 (CEST) Received: by mail-lf1-f53.google.com with SMTP id 2adb3069b0e04-53567b4c3f4so531314e87.2 for ; Tue, 10 Sep 2024 04:05:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1725966331; x=1726571131; 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=0ry14HvN/7lhO/xZUVVWi5sXFLDo4ugqVG52K1fcdXw=; b=DTKvABHYpQTJXkScrbDrcbQtd5cN1AgKJQhaS9sY5wmsNxUGvzJjsoalFCETjhiB1b 22Z1nLBCoIMJ4h7mFB7Oa2AWX0blF3qam7T9J4qY9RTc+qi6UWDCyWPvv0wOu41coUKF eCLzuPYrl75vbTJUCIKRt0d+5GKB5VW7xnAAU+rr3KLXeZ02MUhWHEp8jkPMIngwOtyz UOkiei+vziFdzG2OULtLF9gVADpIk/1rygMWg3/TIq8Qg34OMKA15sjTXMD1TZBPojSq 3Q+nqIo5a85HsoxQYkshU1nfeewc97JhxulFwucDqEYKvsBx2HjKWg5qfN8UwHIfNGUr 8jTQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1725966331; x=1726571131; 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=0ry14HvN/7lhO/xZUVVWi5sXFLDo4ugqVG52K1fcdXw=; b=Aa/YiG+mvjiNRL27x3EVDJyjzoPBUksd9qSMhtMJRyM1O7lqRBNlUORoV8lZZVehxz +pFil5D5NzeRJkEB8RbcjXM0A2O7Em1C2tqZo+guKZj2bYH90ZSHXhibiG04J69/BP5Z IiPvp35DrlwXVsYoH2sh+Hza52LkfAeyvfCVqhXfpl9NJVB6SA+v80skLW2VDroISPAj JlE4eQcTETSs0GATkYvQzoM7BlId7GlbcaC8cVtSBh4IJjLHzbLS/lKICfKCYWq7Y25U JtDuGTO3OlvwHCzYI9SBS4bldotQP6BgJoiNrw8e7S7QNr/ai9d40GpZOe3hQrWRs/dP Y/Qw== X-Forwarded-Encrypted: i=1; AJvYcCVoD9606tATjLJgmsaDBcuDRyktikvI5eqNHwhiakXhmSqaaC2Hcy9ZMNW3i0p8F37rIN8=@dpdk.org X-Gm-Message-State: AOJu0YyyQn2KvGHsqRbbgij2GiRO7TxrRrdMlQE/J7ZCHw71UHh+Z3te ZdM1eLcRawpbMvBHrzB0mp2/3oRwknkUh3qZUyOi37OPH7QjoLhAzhQmdFHqArI= X-Google-Smtp-Source: AGHT+IEijEepT88i3px/hW33QFuDY35MsBQNkscmtbKwzftP+fJkaLljkEeHTBg4w0PflI6z4j/whQ== X-Received: by 2002:a05:6512:238b:b0:536:5646:251e with SMTP id 2adb3069b0e04-536587a34d3mr10346663e87.10.1725966331011; Tue, 10 Sep 2024 04:05:31 -0700 (PDT) Received: from [10.12.0.236] (81.89.53.154.host.vnet.sk. [81.89.53.154]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-a8d25ceaf4fsm465031166b.152.2024.09.10.04.05.29 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 10 Sep 2024 04:05:30 -0700 (PDT) Message-ID: <56ed5ebe-f040-42ea-a6d9-2ab582ff5bf2@pantheon.tech> Date: Tue, 10 Sep 2024 13:05:29 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v1 3/3] dts: rework test suite and dts runner to include test_run configs To: Jeremy Spewock , Nicholas Pratte Cc: yoan.picchi@foss.arm.com, luca.vizzarro@arm.com, probb@iol.unh.edu, paul.szczepanek@arm.com, Honnappa.Nagarahalli@arm.com, dmarx@iol.unh.edu, alex.chapman@arm.com, dev@dpdk.org References: <20240821184305.28028-2-npratte@iol.unh.edu> <20240821184305.28028-5-npratte@iol.unh.edu> 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 4. 9. 2024 20:18, Jeremy Spewock wrote: > On Wed, Aug 21, 2024 at 2:43 PM Nicholas Pratte wrote: > >> diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py >> index 694b2eba65..fd51796a06 100644 >> --- a/dts/framework/test_suite.py >> +++ b/dts/framework/test_suite.py >> @@ -20,6 +20,7 @@ >> from scapy.layers.l2 import Ether # type: ignore[import-untyped] >> from scapy.packet import Packet, Padding # type: ignore[import-untyped] >> >> +from framework.config import TestRunConfiguration >> from framework.testbed_model.port import Port, PortLink >> from framework.testbed_model.sut_node import SutNode >> from framework.testbed_model.tg_node import TGNode >> @@ -64,6 +65,7 @@ class TestSuite: > >> def _process_links(self) -> None: >> """Construct links between SUT and TG ports.""" >> - for sut_port in self.sut_node.ports: >> - for tg_port in self.tg_node.ports: >> - if (sut_port.identifier, sut_port.peer) == ( > > It might be better to squash this commit into the previous just > because this line will cause an error in the previous commit due to > the removal of the identifier and peer attributes. While it is easier > to read broken apart, squashing saves the history from having a > "broken" commit. > Yes, this should be squashed. I don't think there's a clean way to split the commit without breaking the functionality. >> - tg_port.peer, >> - tg_port.identifier, >> - ): >> - self._port_links.append(PortLink(sut_port=sut_port, tg_port=tg_port)) >> + sut_ports = [] >> + for port in self.sut_node.ports: >> + if port.name in [ >> + sut_port.name for sut_port in self.test_run_config.system_under_test_node.ports >> + ]: > > I'm not sure I understand what this check is doing fully. You're > looping through all ports in the SUT's list of ports, and then you are > checking that the name of that port exists in the configuration for > the SUT node in the test run, but aren't the list of ports from the > testrun config going to be the same as the ones from self.sut_node? > The list of ports in self.sut_node is created from the list of ports > that is in the NodeConfiguration, so as long as self.sut_node is the > node that is currently being used in the test run, which should be > handled elsewhere, this will always be True I think. Correct me if I > am misunderstanding though. > > I think what you might be trying to do is access the > `system_under_test_node` field in `test_run` inside of conf.yaml, but > `self.test_run_config.system_under_test_node` does not point to that, > it points to the configuration of the SUT node from `nodes` in > conf.yaml. That would make sense since we really want to limit the > test suites to only having access to the ports that are listed in the > test_run configuration, but if you have only 2 ports in the test_run > configuration with this series applied and 3 in the node > configuration, this list will contain all 3 ports on the node. Maybe > something you could do to solve this is adding `sut_ports` and > `tg_ports` attributes to the TestRunConfiguration and only adding > ports to the test suite if they are in those lists. Admittedly, the > fact that `self.test_run_config.system_under_test_node` is named the > same as something in conf.yaml but points to a different thing than > that key in conf.yaml is pretty confusing. I had to do a couple > double-takes and look through the code path for making these config > classes myself to make sure this was doing what I thought it was. > Maybe we should rename this attribute in the TestRunConfiguration to > be something more like `sut_config` so it is more clear it is pointing > to the configuration of the whole SUT node. > You raise all of the important points. They way config is done now, we lose access to the subset of ports defined in test_run.system_under_test_node.test_bed (which is what the code was likely trying to access). We should have the subset somewhere in the config. The two new attributes make sense since they already mirror what we already have with vdevs, but maybe we could do it some other way. As to the confusion, the two attributes are defined as this: system_under_test_node: SutNodeConfiguration traffic_generator_node: TGNodeConfiguration The name doesn't reflect what it's storing very well and [sut|tg]_config would be a better name, altough it could still be confusing, since it doesn't match the config. Maybe we could add a dict that would mirror the structure of the user config: sut_config: TestRunSutConfigDict Where TestRunSutConfigDict would be class TestRunSutConfigDict(TypedDict): #: Node configuration node_conf: SutNodeConfiguration #: The ports specified in test run config ports: list[str] #: The vdevs specified in test run config vdevs: list[str] This would mirror the config (except node_name became node_conf). Not sure what's best. >> + sut_ports.append(port) >> + tg_ports = [] >> + for port in self.tg_node.ports: >> + if port.name in [ >> + tg_port.name for tg_port in self.test_run_config.traffic_generator_node.ports >> + ]: >> + tg_ports.append(port) >> + >> + # Both the TG and SUT nodes will have an equal number of ports. >> + for i in range(len(sut_ports)): >> + self._port_links.append(PortLink(sut_ports[i], tg_ports[i])) >> >> def set_up_suite(self) -> None: >> """Set up test fixtures common to all test cases. >> -- >> 2.44.0 >>