| Message ID | 20240820004217.1087392-1-kuba@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [net-next,v2] selftests: net: add helper for checking if nettest is available | expand |
On Mon, Aug 19, 2024 at 05:42:17PM -0700, Jakub Kicinski wrote: > diff --git a/tools/testing/selftests/net/settings b/tools/testing/selftests/net/settings > index ed8418e8217a..a38764182822 100644 > --- a/tools/testing/selftests/net/settings > +++ b/tools/testing/selftests/net/settings > @@ -1 +1,2 @@ > timeout=3600 > +profile=1 Excuse me, what's profile used here? I can't find the definition in Documentation/dev-tools/kselftest.rst. Thanks Hangbin
On Mon, Aug 19, 2024 at 05:42:17PM -0700, Jakub Kicinski wrote: > A few tests check if nettest exists in the $PATH before adding > $PWD to $PATH and re-checking. They don't discard stderr on > the first check (and nettest is built as part of selftests, > so it's pretty normal for it to not be available in system $PATH). > This leads to output noise: > > which: no nettest in (/home/virtme/tools/fs/bin:/home/virtme/tools/fs/sbin:/home/virtme/tools/fs/usr/bin:/home/virtme/tools/fs/usr/sbin:/bin:/sbin:/usr/bin:/usr/sbin:/usr/local/bin) > > Add a common helper for the check which does silence stderr. > > There is another small functional change hiding here, because pmtu.sh > and fib_rule_tests.sh used to return from the test case rather than > completely exit. Building nettest is not hard, there should be no need > to maintain the ability to selectively skip cases in its absence. > > Signed-off-by: Jakub Kicinski <kuba@kernel.org> Like Hangbin I am also not sure what "profile=1" is about, but looks fine otherwise, so: Reviewed-by: Ido Schimmel <idosch@nvidia.com> Thanks!
On Tue, 20 Aug 2024 11:18:47 +0800 Hangbin Liu wrote: > Excuse me, what's profile used here? I can't find the definition in > Documentation/dev-tools/kselftest.rst. Ah, sorry, I added timestamping output as a local patch for NIPA. Random example: TAP version 13 1..1 # overriding timeout to 7200 # selftests: net: amt.sh # 13.15 [+13.15] TEST: amt discovery [ OK ] # 16.27 [+3.12] TEST: IPv4 amt multicast forwarding [ OK ] # 19.14 [+2.86] TEST: IPv6 amt multicast forwarding [ OK ] # 670.88 [+651.74] TEST: IPv4 amt traffic forwarding torture [ OK ] # 1203.28 [+532.40] TEST: IPv6 amt traffic forwarding torture [ OK ] ok 1 selftests: net: amt.sh It's not great, makes the lines longer and misaligned. But it's helpful for debugging slow tests.
On Tue, 20 Aug 2024 08:44:12 -0700 Jakub Kicinski wrote: > On Tue, 20 Aug 2024 11:18:47 +0800 Hangbin Liu wrote: > > Excuse me, what's profile used here? I can't find the definition in > > Documentation/dev-tools/kselftest.rst. > > Ah, sorry, I added timestamping output as a local patch for NIPA. To be clear the profile=1 enables the timestamps from my local patch > Random example: > > TAP version 13 > 1..1 > # overriding timeout to 7200 > # selftests: net: amt.sh > # 13.15 [+13.15] TEST: amt discovery [ OK ] > # 16.27 [+3.12] TEST: IPv4 amt multicast forwarding [ OK ] > # 19.14 [+2.86] TEST: IPv6 amt multicast forwarding [ OK ] > # 670.88 [+651.74] TEST: IPv4 amt traffic forwarding torture [ OK ] > # 1203.28 [+532.40] TEST: IPv6 amt traffic forwarding torture [ OK ] > ok 1 selftests: net: amt.sh > > It's not great, makes the lines longer and misaligned. > But it's helpful for debugging slow tests.
On Tue, Aug 20, 2024 at 06:21:47PM -0700, Jakub Kicinski wrote: > On Tue, 20 Aug 2024 08:44:12 -0700 Jakub Kicinski wrote: > > On Tue, 20 Aug 2024 11:18:47 +0800 Hangbin Liu wrote: > > > Excuse me, what's profile used here? I can't find the definition in > > > Documentation/dev-tools/kselftest.rst. > > > > Ah, sorry, I added timestamping output as a local patch for NIPA. > > To be clear the profile=1 enables the timestamps from my local patch I think it's a good feature, maybe you can put it in mainline :) Thanks Hangbin
diff --git a/tools/testing/selftests/net/fcnal-test.sh b/tools/testing/selftests/net/fcnal-test.sh index 386ebd829df5..899dbad0104b 100755 --- a/tools/testing/selftests/net/fcnal-test.sh +++ b/tools/testing/selftests/net/fcnal-test.sh @@ -4304,14 +4304,7 @@ elif [ "$TESTS" = "ipv6" ]; then TESTS="$TESTS_IPV6" fi -# nettest can be run from PATH or from same directory as this selftest -if ! which nettest >/dev/null; then - PATH=$PWD:$PATH - if ! which nettest >/dev/null; then - echo "'nettest' command not found; skipping tests" - exit $ksft_skip - fi -fi +check_gen_prog "nettest" declare -i nfail=0 declare -i nsuccess=0 diff --git a/tools/testing/selftests/net/fib_rule_tests.sh b/tools/testing/selftests/net/fib_rule_tests.sh index 89034c5b69dc..53c5c1ad437e 100755 --- a/tools/testing/selftests/net/fib_rule_tests.sh +++ b/tools/testing/selftests/net/fib_rule_tests.sh @@ -51,31 +51,6 @@ log_test() fi } -check_nettest() -{ - if which nettest > /dev/null 2>&1; then - return 0 - fi - - # Add the selftest directory to PATH if not already done - if [ "${SELFTEST_PATH}" = "" ]; then - SELFTEST_PATH="$(dirname $0)" - PATH="${PATH}:${SELFTEST_PATH}" - - # Now retry with the new path - if which nettest > /dev/null 2>&1; then - return 0 - fi - - if [ "${ret}" -eq 0 ]; then - ret="${ksft_skip}" - fi - echo "nettest not found (try 'make -C ${SELFTEST_PATH} nettest')" - fi - - return 1 -} - setup() { set -e @@ -317,11 +292,6 @@ fib_rule6_connect_test() echo echo "IPv6 FIB rule connect tests" - if ! check_nettest; then - echo "SKIP: Could not run test without nettest tool" - return - fi - setup_peer $IP -6 rule add dsfield 0x04 table $RTABLE_PEER @@ -516,11 +486,6 @@ fib_rule4_connect_test() echo echo "IPv4 FIB rule connect tests" - if ! check_nettest; then - echo "SKIP: Could not run test without nettest tool" - return - fi - setup_peer $IP -4 rule add dsfield 0x04 table $RTABLE_PEER @@ -584,6 +549,8 @@ if [ ! -x "$(command -v ip)" ]; then exit $ksft_skip fi +check_gen_prog "nettest" + # start clean cleanup &> /dev/null setup diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh index 8ee4489238ca..be8707bfb46e 100644 --- a/tools/testing/selftests/net/lib.sh +++ b/tools/testing/selftests/net/lib.sh @@ -125,6 +125,21 @@ slowwait_for_counter() slowwait "$timeout" until_counter_is ">= $((base + delta))" "$@" } +# Check for existence of tools which are built as part of selftests +# but may also already exist in $PATH +check_gen_prog() +{ + local prog_name=$1; shift + + if ! which $prog_name >/dev/null 2>/dev/null; then + PATH=$PWD:$PATH + if ! which $prog_name >/dev/null; then + echo "'$prog_name' command not found; skipping tests" + exit $ksft_skip + fi + fi +} + remove_ns_list() { local item=$1 diff --git a/tools/testing/selftests/net/pmtu.sh b/tools/testing/selftests/net/pmtu.sh index 24a50622406c..569bce8b6383 100755 --- a/tools/testing/selftests/net/pmtu.sh +++ b/tools/testing/selftests/net/pmtu.sh @@ -681,13 +681,7 @@ setup_xfrm() { } setup_nettest_xfrm() { - if ! which nettest >/dev/null; then - PATH=$PWD:$PATH - if ! which nettest >/dev/null; then - echo "'nettest' command not found; skipping tests" - return 1 - fi - fi + check_gen_prog "nettest" [ ${1} -eq 6 ] && proto="-6" || proto="" port=${2} diff --git a/tools/testing/selftests/net/settings b/tools/testing/selftests/net/settings index ed8418e8217a..a38764182822 100644 --- a/tools/testing/selftests/net/settings +++ b/tools/testing/selftests/net/settings @@ -1 +1,2 @@ timeout=3600 +profile=1 diff --git a/tools/testing/selftests/net/unicast_extensions.sh b/tools/testing/selftests/net/unicast_extensions.sh index f52aa5f7da52..3e751234ccfe 100755 --- a/tools/testing/selftests/net/unicast_extensions.sh +++ b/tools/testing/selftests/net/unicast_extensions.sh @@ -30,14 +30,7 @@ source lib.sh -# nettest can be run from PATH or from same directory as this selftest -if ! which nettest >/dev/null; then - PATH=$PWD:$PATH - if ! which nettest >/dev/null; then - echo "'nettest' command not found; skipping tests" - exit $ksft_skip - fi -fi +check_gen_prog "nettest" result=0 diff --git a/tools/testing/selftests/net/vrf_route_leaking.sh b/tools/testing/selftests/net/vrf_route_leaking.sh index 152171fb1fc8..e9c2f71da207 100755 --- a/tools/testing/selftests/net/vrf_route_leaking.sh +++ b/tools/testing/selftests/net/vrf_route_leaking.sh @@ -59,7 +59,6 @@ # while it is forwarded between different vrfs. source lib.sh -PATH=$PWD:$PWD/tools/testing/selftests/net:$PATH VERBOSE=0 PAUSE_ON_FAIL=no DEFAULT_TTYPE=sym @@ -636,6 +635,8 @@ EOF # Some systems don't have a ping6 binary anymore command -v ping6 > /dev/null 2>&1 && ping6=$(command -v ping6) || ping6=$(command -v ping) +check_gen_prog "nettest" + TESTS_IPV4="ipv4_ping_ttl ipv4_traceroute ipv4_ping_frag ipv4_ping_local ipv4_tcp_local ipv4_udp_local ipv4_ping_ttl_asym ipv4_traceroute_asym" TESTS_IPV6="ipv6_ping_ttl ipv6_traceroute ipv6_ping_local ipv6_tcp_local ipv6_udp_local
A few tests check if nettest exists in the $PATH before adding $PWD to $PATH and re-checking. They don't discard stderr on the first check (and nettest is built as part of selftests, so it's pretty normal for it to not be available in system $PATH). This leads to output noise: which: no nettest in (/home/virtme/tools/fs/bin:/home/virtme/tools/fs/sbin:/home/virtme/tools/fs/usr/bin:/home/virtme/tools/fs/usr/sbin:/bin:/sbin:/usr/bin:/usr/sbin:/usr/local/bin) Add a common helper for the check which does silence stderr. There is another small functional change hiding here, because pmtu.sh and fib_rule_tests.sh used to return from the test case rather than completely exit. Building nettest is not hard, there should be no need to maintain the ability to selectively skip cases in its absence. Signed-off-by: Jakub Kicinski <kuba@kernel.org> --- v2: - fold in the changes from Ido v1: https://lore.kernel.org/20240817183848.658443-1-kuba@kernel.org CC: shuah@kernel.org CC: linux-kselftest@vger.kernel.org CC: Ido Schimmel <idosch@idosch.org> --- tools/testing/selftests/net/fcnal-test.sh | 9 +---- tools/testing/selftests/net/fib_rule_tests.sh | 37 +------------------ tools/testing/selftests/net/lib.sh | 15 ++++++++ tools/testing/selftests/net/pmtu.sh | 8 +--- tools/testing/selftests/net/settings | 1 + .../selftests/net/unicast_extensions.sh | 9 +---- .../selftests/net/vrf_route_leaking.sh | 3 +- 7 files changed, 23 insertions(+), 59 deletions(-)