On 4. 9. 2024 20:18, Jeremy Spewock wrote:
On Wed, Aug 21, 2024 at 2:43 PM Nicholas Pratte <npra...@iol.unh.edu> wrote:
<snip>
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:
<snip>
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
