Re: [PATCH v1 3/3] dts: rework test suite and dts runner to include test_run configs

[Jeremy Spewock <jspewock@iol.unh.edu>]

On Wed, Aug 21, 2024 at 2:43 PM Nicholas Pratte <npratte@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.

> -                    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.

> +                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
>