Re: [PATCHv3 net-next 3/3] tools: ynl: add YNL test framework

[Donald Hunter <donald.hunter@gmail.com>]

Hangbin Liu <liuhangbin@gmail.com> writes:

> Add a test framework for YAML Netlink (YNL) tools, covering both CLI and
> ethtool functionality. The framework includes:
>
> 1) cli: family listing, netdev, ethtool, rt-* families, and nlctrl
>    operations
> 2) ethtool: device info, statistics, ring/coalesce/pause parameters, and
>    feature gettings
>
> The current YNL syntax is a bit obscure, and end users may not always know
> how to use it. This test framework provides usage examples and also serves
> as a regression test to catch potential breakages caused by future changes.
>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  tools/net/ynl/Makefile                  |   8 +-
>  tools/net/ynl/tests/Makefile            |  38 ++++
>  tools/net/ynl/tests/config              |   6 +
>  tools/net/ynl/tests/test_ynl_cli.sh     | 291 ++++++++++++++++++++++++
>  tools/net/ynl/tests/test_ynl_ethtool.sh | 196 ++++++++++++++++
>  5 files changed, 537 insertions(+), 2 deletions(-)
>  create mode 100644 tools/net/ynl/tests/Makefile
>  create mode 100644 tools/net/ynl/tests/config
>  create mode 100755 tools/net/ynl/tests/test_ynl_cli.sh
>  create mode 100755 tools/net/ynl/tests/test_ynl_ethtool.sh
>
> diff --git a/tools/net/ynl/Makefile b/tools/net/ynl/Makefile
> index 211df5a93ad9..8a328972564a 100644
> --- a/tools/net/ynl/Makefile
> +++ b/tools/net/ynl/Makefile
> @@ -12,7 +12,7 @@ endif
>  libdir  ?= $(prefix)/$(libdir_relative)
>  includedir ?= $(prefix)/include
>  
> -SUBDIRS = lib generated samples
> +SUBDIRS = lib generated samples tests
>  
>  all: $(SUBDIRS) libynl.a
>  
> @@ -48,5 +48,9 @@ install: libynl.a lib/*.h
>  	@echo -e "\tINSTALL pyynl"
>  	@pip install --prefix=$(DESTDIR)$(prefix) .
>  	@make -C generated install
> +	@make -C tests install
>  
> -.PHONY: all clean distclean install $(SUBDIRS)
> +run_tests:
> +	@$(MAKE) -C tests run_tests
> +
> +.PHONY: all clean distclean install run_tests $(SUBDIRS)
> diff --git a/tools/net/ynl/tests/Makefile b/tools/net/ynl/tests/Makefile
> new file mode 100644
> index 000000000000..4d527f9c3de9
> --- /dev/null
> +++ b/tools/net/ynl/tests/Makefile
> @@ -0,0 +1,38 @@
> +# SPDX-License-Identifier: GPL-2.0
> +# Makefile for YNL tests
> +
> +TESTS := \
> +	test_ynl_cli.sh \
> +	test_ynl_ethtool.sh \
> +# end of TESTS
> +
> +all: $(TESTS)
> +
> +run_tests:
> +	@echo "Running YNL tests..."
> +	@failed=0; \
> +	echo "Running test_ynl_cli.sh..."; \
> +	./test_ynl_cli.sh || failed=$$(($$failed + 1)); \
> +	echo "Running test_ynl_ethtool.sh..."; \
> +	./test_ynl_ethtool.sh || failed=$$(($$failed + 1)); \

This could iterate through $(TESTS) instead of being hard coded.

> +	if [ $$failed -eq 0 ]; then \
> +		echo "All tests passed!"; \
> +	else \
> +		echo "$$failed test(s) failed!"; \

AFAICS this will never be reported since the scripts only ever exit 0.
The message is also a bit misleading since it would be the count of
scripts that failed, not individual tests.

It would be great if the scripts exited with the number of test failures
so the make file could report a total.

> +		exit 1; \
> +	fi
> +
> +install: $(TESTS)
> +	@mkdir -p $(DESTDIR)/usr/bin
> +	@for test in $(TESTS); do \
> +		name=$$(basename $$test .sh); \
> +		sed -e 's|^ynl=.*|ynl="ynl"|' \
> +		    -e 's|^ynl_ethtool=.*|ynl_ethtool="ynl-ethtool"|' \
> +		    $$test > $(DESTDIR)/usr/bin/$$name; \
> +		chmod +x $(DESTDIR)/usr/bin/$$name; \
> +	done
> +
> +clean:
> +	@# Nothing to clean
> +
> +.PHONY: all install clean run_tests
> diff --git a/tools/net/ynl/tests/config b/tools/net/ynl/tests/config
> new file mode 100644
> index 000000000000..339f1309c03f
> --- /dev/null
> +++ b/tools/net/ynl/tests/config
> @@ -0,0 +1,6 @@
> +CONFIG_DUMMY=m
> +CONFIG_INET_DIAG=y
> +CONFIG_IPV6=y
> +CONFIG_NET_NS=y
> +CONFIG_NETDEVSIM=m
> +CONFIG_VETH=m
> diff --git a/tools/net/ynl/tests/test_ynl_cli.sh b/tools/net/ynl/tests/test_ynl_cli.sh
> new file mode 100755
> index 000000000000..5cc0624ffaad
> --- /dev/null
> +++ b/tools/net/ynl/tests/test_ynl_cli.sh
> @@ -0,0 +1,291 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Test YNL CLI functionality
> +
> +# Default ynl path for direct execution, can be overridden by make install
> +ynl="../pyynl/cli.py"
> +
> +readonly NSIM_ID="1338"
> +readonly NSIM_DEV_NAME="nsim${NSIM_ID}"
> +readonly VETH_A="veth_a"
> +readonly VETH_B="veth_b"
> +
> +testns="ynl-$(mktemp -u XXXXXX)"
> +
> +# Test listing available families
> +cli_list_families() {
> +	if $ynl --list-families &>/dev/null; then
> +		echo "PASS: YNL CLI list families"
> +	else
> +		echo "FAIL: YNL CLI list families"
> +	fi
> +}
> +
> +# Test netdev family operations (dev-get, queue-get)
> +cli_netdev_ops() {
> +	local dev_output
> +	local ifindex
> +
> +	ifindex=$(ip netns exec "$testns" cat /sys/class/net/"$NSIM_DEV_NAME"/ifindex 2>/dev/null)
> +	if [[ -z "$ifindex" ]]; then
> +		echo "FAIL: YNL CLI netdev operations (failed to get ifindex)"

This is a bit misleading, there's no ynl command here so I don't think
it should be a FAIL. Can we just report SKIP when it is an infra issue?

> +		return
> +	fi
> +
> +	dev_output=$(ip netns exec "$testns" $ynl --family netdev \
> +		--do dev-get --json "{\"ifindex\": $ifindex}" 2>/dev/null)
> +
> +	if ! echo "$dev_output" | grep -q "ifindex"; then
> +		echo "FAIL: YNL CLI netdev operations (netdev dev-get output missing ifindex)"
> +		return
> +	fi
> +
> +	if ! ip netns exec "$testns" $ynl --family netdev \
> +		--dump queue-get --json "{\"ifindex\": $ifindex}" &>/dev/null; then
> +		echo "FAIL: YNL CLI netdev operations (failed to get netdev queue info)"
> +		return
> +	fi
> +
> +	echo "PASS: YNL CLI netdev operations"
> +}
> +
> +# Test ethtool family operations (rings-get, linkinfo-get)
> +cli_ethtool_ops() {
> +	local rings_output
> +	local linkinfo_output
> +
> +	rings_output=$(ip netns exec "$testns" $ynl --family ethtool \
> +		--do rings-get --json "{\"header\": {\"dev-name\": \"$NSIM_DEV_NAME\"}}" 2>/dev/null)
> +
> +	if ! echo "$rings_output" | grep -q "header"; then
> +		echo "FAIL: YNL CLI ethtool operations (ethtool rings-get output missing header)"
> +		return
> +	fi
> +
> +	linkinfo_output=$(ip netns exec "$testns" $ynl --family ethtool \
> +		--do linkinfo-get --json "{\"header\": {\"dev-name\": \"$VETH_A\"}}" 2>/dev/null)
> +
> +	if ! echo "$linkinfo_output" | grep -q "header"; then
> +		echo "FAIL: YNL CLI ethtool operations (ethtool linkinfo-get output missing header)"
> +		return
> +	fi
> +
> +	echo "PASS: YNL CLI ethtool operations"
> +}
> +
> +# Test rt-* family operations (route, addr, link, neigh, rule)
> +cli_rt_ops() {
> +	local ifindex
> +
> +	if ! $ynl --list-families 2>/dev/null | grep -q "rt-"; then
> +		echo "SKIP: YNL CLI rt-* operations (no rt-* families available)"
> +		return
> +	fi
> +
> +	ifindex=$(ip netns exec "$testns" cat /sys/class/net/"$NSIM_DEV_NAME"/ifindex 2>/dev/null)
> +	if [[ -z "$ifindex" ]]; then
> +		echo "FAIL: YNL CLI rt-* operations (failed to get ifindex)"

Also FAIL -> SKIP ?

> +		return
> +	fi