Re: [PATCH] test: pytest: introduce pytest for network, test tftp notfound

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hello Jonas,

Thanks for your patch!

Some minor comments below.

On 05.06.24 15:13, Jonas Rebmann wrote:
> barebox already has a few pytest integration tests but none of them are
> networking related. This type of test requires a network connection to
> the barebox device, without the "network" feature the test is marked
> xfail. DHCP is assumed. For the case of qemu device we also assume DHCP
> to set serverip correctly. In any other case, it is set via console to
> that IP of the pytest-host that has the route to the barebox devices IP.
> The test asserts on a successful ICMP ping from the test device to the
> pytest-host as well as on a "No such file or directory" conversation
> from the barebox device to the pytest-host

Can you add some line breaks into the commit message for v2?

Also please add following paragraph (or something similar):

  Testing with real hardware and LG_PROXY would fail, but that's arguably
  ok for now.

Just to document the limitation.

> Signed-off-by: Jonas Rebmann <mail@schlaraffenlan.de>
> ---
>  test/arm/virt@multi_v8_defconfig.yaml |  1 +
>  test/openrisc/generic_defconfig.yaml  |  2 +

I wouldn't have expected that so few defconfigs have networking
out of the box. Once we have this first test, it'll be worthwhile
to add support for the other targets too.

>  test/py/test_network.py               | 69 +++++++++++++++++++++++++++++++++++
>  3 files changed, 72 insertions(+)
> 
> diff --git a/test/arm/virt@multi_v8_defconfig.yaml b/test/arm/virt@multi_v8_defconfig.yaml
> index d8f8ab5cbf..42ce10328d 100644
> --- a/test/arm/virt@multi_v8_defconfig.yaml
> +++ b/test/arm/virt@multi_v8_defconfig.yaml
> @@ -14,6 +14,7 @@ targets:
>        BareboxTestStrategy: {}
>      features:
>        - virtio-mmio
> +      - network
>      runner:
>        tuxmake_arch: arm64
>  images:
> diff --git a/test/openrisc/generic_defconfig.yaml b/test/openrisc/generic_defconfig.yaml
> index 93ba9586c4..56b70b8242 100644
> --- a/test/openrisc/generic_defconfig.yaml
> +++ b/test/openrisc/generic_defconfig.yaml
> @@ -12,6 +12,8 @@ targets:
>          prompt: 'barebox@[^:]+:[^ ]+ '
>          bootstring: 'commandline:'
>        BareboxTestStrategy: {}
> +    features:
> +      - network
>  images:
>    barebox: !template "$LG_BUILDDIR/barebox"
>  imports:
> diff --git a/test/py/test_network.py b/test/py/test_network.py
> new file mode 100644
> index 0000000000..00bcea3606
> --- /dev/null
> +++ b/test/py/test_network.py
> @@ -0,0 +1,69 @@
> +import pytest
> +
> +from labgrid import driver,Environment
> +import socket
> +import threading
> +
> +TFTP_TEST_PORT = 6968

Hmm, hardcoding this is a bit unfortunate as it could lead to tests for multiple DUTs
running in parallel to interfere with each other.

> +
> +def get_source_addr(destination_ip, destination_port):
> +    udp_socket = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
> +    udp_socket.connect((destination_ip, destination_port))
> +    source_ip, _ = udp_socket.getsockname()
> +    return source_ip
> +
> +def expect_tftp_notfound(filename, listen_port, listen_addr):
> +    uname = filename.encode("ascii")
> +    messages = [
> +        b"\000\001" + uname + b"\000octet\000timeout\0005\000blksize\000512\000tsize\0000\000",
> +        b"\000\005\000\001File not found\000",
> +    ]
> +    udp_socket = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
> +    udp_socket.bind((listen_addr, listen_port))

If you set listen_port to 0, you get an ephemeral port assigned to avoid
port number clashes.
You'll probably want to move this out of the thread then.

> +    udp_socket.settimeout(3)
> +    data, addr = udp_socket.recvfrom(1024)
> +    udp_socket.sendto(messages[1], addr)
> +    udp_socket.close()
> +    assert data == messages[0]
> +
> +def test_skip():
> +    pytest.mark.skip("no network adapter available")

Unused.

> +def test_barebox_tftp_not_found(barebox, barebox_config, env):
> +    # on duts without network feature, this is expected to fail
> +    # set xfail_strict=True to enforce specifying the network feature if available
> +    if not 'network' in env.get_target_features():
> +        pytest.xfail("network feature not specified")

Nice. Didn't know about xfail.

> +    barebox.run_check("ifup eth0")

I don't think we have any such boards in Qemu, but in general boards
can have multiple Ethernet ports and the first might not even
have a link. Please use ifup -a instead to bring up all interfaces.
If multiple ports have a link, maybe test on all of them, e.g.:

for line in stdout:
    m = re.search('DHCP client bound to address (.*)', line)
    if m is None:
        continue
 
    do_what_you_currently_do(address=m.group(1))

> +    guestaddr = barebox.run_check("echo $eth0.ipaddr")[0]
> +    assert guestaddr != "0.0.0.0"
> +
> +    listen_addr = "127.0.0.1"
> +    if not isinstance(barebox.console, driver.QEMUDriver):
> +        # sending an arbitrary udp package to determine the IP towards dut
> +        listen_addr = get_source_addr(guestaddr, TFTP_TEST_PORT)
> +        barebox.run_check(f"eth0.serverip={listen_addr}")
> +
> +    barebox.run_check("ping $eth0.serverip", timeout=2)
> +
> +    tftp_thread = threading.Thread(
> +        target=expect_tftp_notfound,
> +        name="expect_tftp_notfound",
> +        args=("a", TFTP_TEST_PORT, listen_addr),
> +    )
> +    tftp_thread.daemon = True
> +    tftp_thread.start()
> +    
> +    try:
> +        stdout, _, returncode = barebox.run(f"tftp -P {TFTP_TEST_PORT} a", timeout=3)
> +        assert returncode == 127

returncode != 0. We don't care which error code.

> +    except:
> +        raise

This is unneeded.

> +    finally:
> +        # terminate a timed-out ftpf

s/ftpf/tftp/

> +        barebox.console.sendcontrol("c")
> +
> +    tftp_thread.join()
> +    barebox.run_check("ifdown eth0")

These should be moved into the finally clause too, right?

Cheers,
Ahmad

> +
> 
> ---
> base-commit: 55cf43c442c260846b393c2099d4a72d0ebdbdd9
> change-id: 20240605-test_tftp-9e12282d46a0
> 
> Best regards,

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |