[PATCH net 0/2] selftests: net: add missing tests to Makefile

[PATCH net 0/2] selftests: net: add missing tests to Makefile

[Hangbin Liu]

When generating the selftests to another folder, the fixed tests are
missing as they are not in Makefile. The missing tests are generated
by command:
$ for f in $(ls *.sh); do grep -q $f Makefile || echo $f; done

I think there need a way to notify the developer when they created a new
file in selftests folder. Maybe a bot like bluez.test.bot or kernel
test robot could help do that?

Hangbin Liu (2):
  selftests/net: add missing tests
  selftests/net/forwarding: add missing tests

 tools/testing/selftests/net/Makefile          |  3 +-
 .../testing/selftests/net/forwarding/Makefile | 33 +++++++++++++++++++
 2 files changed, 35 insertions(+), 1 deletion(-)

-- 
2.35.1

[PATCH net 1/2] selftests/net: add missing tests to Makefile

[Hangbin Liu]

When generating the selftests to another folder, the fixed tests are
missing as they are not in Makefile, e.g.

  make -C tools/testing/selftests/ install \
  	TARGETS="net" INSTALL_PATH=/tmp/kselftests

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 tools/testing/selftests/net/Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
index 3fe2515aa616..0f2ebc38d893 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -30,7 +30,7 @@ TEST_PROGS += ioam6.sh
 TEST_PROGS += gro.sh
 TEST_PROGS += gre_gso.sh
 TEST_PROGS += cmsg_so_mark.sh
-TEST_PROGS += cmsg_time.sh
+TEST_PROGS += cmsg_time.sh cmsg_ipv6.sh
 TEST_PROGS += srv6_end_dt46_l3vpn_test.sh
 TEST_PROGS += srv6_end_dt4_l3vpn_test.sh
 TEST_PROGS += srv6_end_dt6_l3vpn_test.sh
@@ -54,6 +54,7 @@ TEST_GEN_PROGS = reuseport_bpf reuseport_bpf_cpu reuseport_bpf_numa
 TEST_GEN_PROGS += reuseport_dualstack reuseaddr_conflict tls
 TEST_GEN_FILES += toeplitz
 TEST_GEN_FILES += cmsg_sender
+TEST_PROGS += test_vxlan_vnifiltering.sh
 
 TEST_FILES := settings
 
-- 
2.35.1

[PATCH net 2/2] selftests/net/forwarding: add missing tests to Makefile

[Hangbin Liu]

When generating the selftests to another folder, the fixed tests are
missing as they are not in Makefile, e.g.

  make -C tools/testing/selftests/ install \
  	TARGETS="net/forwarding" INSTALL_PATH=/tmp/kselftests

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 .../testing/selftests/net/forwarding/Makefile | 33 +++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/tools/testing/selftests/net/forwarding/Makefile b/tools/testing/selftests/net/forwarding/Makefile
index 8fa97ae9af9e..c87e674b61b1 100644
--- a/tools/testing/selftests/net/forwarding/Makefile
+++ b/tools/testing/selftests/net/forwarding/Makefile
@@ -2,15 +2,31 @@
 
 TEST_PROGS = bridge_igmp.sh \
 	bridge_locked_port.sh \
+	bridge_mld.sh \
 	bridge_port_isolation.sh \
 	bridge_sticky_fdb.sh \
 	bridge_vlan_aware.sh \
+	bridge_vlan_mcast.sh \
 	bridge_vlan_unaware.sh \
+	custom_multipath_hash.sh \
+	dual_vxlan_bridge.sh \
+	ethtool_extended_state.sh \
 	ethtool.sh \
+	gre_custom_multipath_hash.sh \
 	gre_inner_v4_multipath.sh \
 	gre_inner_v6_multipath.sh \
+	gre_multipath_nh_res.sh \
+	gre_multipath_nh.sh \
 	gre_multipath.sh \
+	hw_stats_l3.sh \
 	ip6_forward_instats_vrf.sh \
+	ip6gre_custom_multipath_hash.sh \
+	ip6gre_flat_key.sh \
+	ip6gre_flat_keys.sh \
+	ip6gre_flat.sh \
+	ip6gre_hier_key.sh \
+	ip6gre_hier_keys.sh \
+	ip6gre_hier.sh \
 	ip6gre_inner_v4_multipath.sh \
 	ip6gre_inner_v6_multipath.sh \
 	ipip_flat_gre_key.sh \
@@ -34,36 +50,53 @@ TEST_PROGS = bridge_igmp.sh \
 	mirror_gre_vlan_bridge_1q.sh \
 	mirror_gre_vlan.sh \
 	mirror_vlan.sh \
+	pedit_dsfield.sh \
+	pedit_ip.sh \
+	pedit_l4port.sh \
+	q_in_vni_ipv6.sh \
+	q_in_vni.sh \
 	router_bridge.sh \
 	router_bridge_vlan.sh \
 	router_broadcast.sh \
+	router_mpath_nh_res.sh \
 	router_mpath_nh.sh \
 	router_multicast.sh \
 	router_multipath.sh \
+	router_nh.sh \
 	router.sh \
 	router_vid_1.sh \
 	sch_ets.sh \
+	sch_red.sh \
 	sch_tbf_ets.sh \
 	sch_tbf_prio.sh \
 	sch_tbf_root.sh \
+	skbedit_priority.sh \
 	tc_actions.sh \
 	tc_chains.sh \
 	tc_flower_router.sh \
 	tc_flower.sh \
 	tc_mpls_l2vpn.sh \
+	tc_police.sh \
 	tc_shblocks.sh \
 	tc_vlan_modify.sh \
+	vxlan_asymmetric_ipv6.sh \
 	vxlan_asymmetric.sh \
+	vxlan_bridge_1d_ipv6.sh \
+	vxlan_bridge_1d_port_8472_ipv6.sh \
 	vxlan_bridge_1d_port_8472.sh \
 	vxlan_bridge_1d.sh \
+	vxlan_bridge_1q_ipv6.sh \
+	vxlan_bridge_1q_port_8472_ipv6.sh
 	vxlan_bridge_1q_port_8472.sh \
 	vxlan_bridge_1q.sh \
+	vxlan_symmetric_ipv6.sh \
 	vxlan_symmetric.sh
 
 TEST_PROGS_EXTENDED := devlink_lib.sh \
 	ethtool_lib.sh \
 	fib_offload_lib.sh \
 	forwarding.config.sample \
+	ip6gre_lib.sh \
 	ipip_lib.sh \
 	lib.sh \
 	mirror_gre_lib.sh \
-- 
2.35.1

Re: [PATCH net 0/2] selftests: net: add missing tests to Makefile

[Jakub Kicinski]

On Thu, 28 Apr 2022 12:45:09 +0800 Hangbin Liu wrote:
> When generating the selftests to another folder, the fixed tests are
> missing as they are not in Makefile. The missing tests are generated
> by command:
> $ for f in $(ls *.sh); do grep -q $f Makefile || echo $f; done
> 
> I think there need a way to notify the developer when they created a new
> file in selftests folder. Maybe a bot like bluez.test.bot or kernel
> test robot could help do that?

Our netdev patch checks are here:

https://github.com/kuba-moo/nipa/tree/master/tests/patch

in case you're willing to code it up and post a PR.

Re: [PATCH net 0/2] selftests: net: add missing tests to Makefile

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 28 Apr 2022 12:45:09 +0800 you wrote:
> When generating the selftests to another folder, the fixed tests are
> missing as they are not in Makefile. The missing tests are generated
> by command:
> $ for f in $(ls *.sh); do grep -q $f Makefile || echo $f; done
> 
> I think there need a way to notify the developer when they created a new
> file in selftests folder. Maybe a bot like bluez.test.bot or kernel
> test robot could help do that?
> 
> [...]

Here is the summary with links:
  - [net,1/2] selftests/net: add missing tests to Makefile
    https://git.kernel.org/netdev/net/c/38dcd9570d6f
  - [net,2/2] selftests/net/forwarding: add missing tests to Makefile
    https://git.kernel.org/netdev/net/c/f62c5acc800e

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net 0/2] selftests: net: add missing tests to Makefile

[Hangbin Liu]

On Fri, Apr 29, 2022 at 05:56:04PM -0700, Jakub Kicinski wrote:
> On Thu, 28 Apr 2022 12:45:09 +0800 Hangbin Liu wrote:
> > When generating the selftests to another folder, the fixed tests are
> > missing as they are not in Makefile. The missing tests are generated
> > by command:
> > $ for f in $(ls *.sh); do grep -q $f Makefile || echo $f; done
> > 
> > I think there need a way to notify the developer when they created a new
> > file in selftests folder. Maybe a bot like bluez.test.bot or kernel
> > test robot could help do that?
> 
> Our netdev patch checks are here:
> 
> https://github.com/kuba-moo/nipa/tree/master/tests/patch
> 
> in case you're willing to code it up and post a PR.

Thanks, I will check this and post MR when have time.

Hangbin

Re: [PATCH net 0/2] selftests: net: add missing tests to Makefile

[Hangbin Liu]

On Fri, Apr 29, 2022 at 05:56:04PM -0700, Jakub Kicinski wrote:
> On Thu, 28 Apr 2022 12:45:09 +0800 Hangbin Liu wrote:
> > I think there need a way to notify the developer when they created a new
> > file in selftests folder. Maybe a bot like bluez.test.bot or kernel
> > test robot could help do that?
> 
> Our netdev patch checks are here:
> 
> https://github.com/kuba-moo/nipa/tree/master/tests/patch
> 
> in case you're willing to code it up and post a PR.

Hi Jakub,

I checked the tools and write a draft patch. But I have a question before post
the PR. AFAIK, This bot is only used for checking patches and adding status in
patchwork. But it doesn't support sending a reply to developer, right?

For the selftest reminder, I think it would be good to let developer know
via email if the file is missing in Makefile. What do you think?

Here is the draft patch:

diff --git a/tests/patch/check_selftest/check_selftest.sh b/tests/patch/check_selftest/check_selftest.sh
new file mode 100755
index 0000000..ad7c608
--- /dev/null
+++ b/tests/patch/check_selftest/check_selftest.sh
@@ -0,0 +1,28 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+rt=0
+if ! git show --name-status --oneline | \
+	grep -P '^A\ttools/testing/selftests/net/' | \
+	grep '\.sh$'; then
+	echo "No new net selftests script" >&$DESC_FD
+	exit 0
+fi
+
+files=$(git show --name-status --oneline | grep -P '^A\ttools/testing/selftests/net/' | grep '\.sh$' | sed 's@A\ttools/testing/selftests/net/@@')
+for file in $files; do
+	if echo $file | grep forwarding; then
+		file=$(echo $file | sed 's/forwarding\///')
+		if ! grep -P "[\t| ]$file" tools/testing/selftests/net/forwarding/Makefile;then
+			echo "new test $file not in selftests/net/forwarding/Makefile" >&$DESC_FD
+			rc=1
+		fi
+	else
+		if ! grep -P "[\t| ]$file" tools/testing/selftests/net/Makefile;then
+			echo "new test $file not in selftests/net/Makefile" >&$DESC_FD
+			rc=1
+		fi
+	fi
+done
+
+exit $rc
diff --git a/tests/patch/check_selftest/info.json b/tests/patch/check_selftest/info.json
new file mode 100644
index 0000000..615779f
--- /dev/null
+++ b/tests/patch/check_selftest/info.json
@@ -0,0 +1,3 @@
+{
+  "run": ["check_selftest.sh"]
+}
-- 
2.35.1

Re: [PATCH net 0/2] selftests: net: add missing tests to Makefile

[Jakub Kicinski]

On Tue, 17 May 2022 14:23:22 +0800 Hangbin Liu wrote:
> On Fri, Apr 29, 2022 at 05:56:04PM -0700, Jakub Kicinski wrote:
> > On Thu, 28 Apr 2022 12:45:09 +0800 Hangbin Liu wrote:  
> > > I think there need a way to notify the developer when they created a new
> > > file in selftests folder. Maybe a bot like bluez.test.bot or kernel
> > > test robot could help do that?  
> > 
> > Our netdev patch checks are here:
> > 
> > https://github.com/kuba-moo/nipa/tree/master/tests/patch
> > 
> > in case you're willing to code it up and post a PR.  
> 
> Hi Jakub,
> 
> I checked the tools and write a draft patch. But I have a question before post
> the PR.

First off - thanks a log for doing this!

> AFAIK, This bot is only used for checking patches and adding status in
> patchwork. But it doesn't support sending a reply to developer, right?
> 
> For the selftest reminder, I think it would be good to let developer know
> via email if the file is missing in Makefile. What do you think?

Yes, we don't have the auto-reply. There's too much noise in some of
the tests, but mostly it's because we don't want to encourage people
posting patches just to build them. If it's a machine replying rather
than a human some may think that it's okay. We already have
jaw-droppingly expensive VM instance to keep up with the build volume.
And the list is very busy. So we can't afford "post to run the CI"
development model.

> Here is the draft patch:
> 
> diff --git a/tests/patch/check_selftest/check_selftest.sh b/tests/patch/check_selftest/check_selftest.sh
> new file mode 100755
> index 0000000..ad7c608
> --- /dev/null
> +++ b/tests/patch/check_selftest/check_selftest.sh
> @@ -0,0 +1,28 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +rt=0
> +if ! git show --name-status --oneline | \
> +	grep -P '^A\ttools/testing/selftests/net/' | \
> +	grep '\.sh$'; then
> +	echo "No new net selftests script" >&$DESC_FD
> +	exit 0
> +fi
> +
> +files=$(git show --name-status --oneline | grep -P '^A\ttools/testing/selftests/net/' | grep '\.sh$' | sed 's@A\ttools/testing/selftests/net/@@')
> +for file in $files; do
> +	if echo $file | grep forwarding; then
> +		file=$(echo $file | sed 's/forwarding\///')
> +		if ! grep -P "[\t| ]$file" tools/testing/selftests/net/forwarding/Makefile;then
> +			echo "new test $file not in selftests/net/forwarding/Makefile" >&$DESC_FD
> +			rc=1
> +		fi
> +	else
> +		if ! grep -P "[\t| ]$file" tools/testing/selftests/net/Makefile;then
> +			echo "new test $file not in selftests/net/Makefile" >&$DESC_FD
> +			rc=1
> +		fi

Does it matter which exact selftest makefile the changes are?
Maybe as a first stab we should just check if there are changes 
to anything in tools/testing/selftests/.*/Makefile?

We can see if there are false-negatives.

> +	fi
> +done
> +
> +exit $rc
> diff --git a/tests/patch/check_selftest/info.json b/tests/patch/check_selftest/info.json
> new file mode 100644
> index 0000000..615779f
> --- /dev/null
> +++ b/tests/patch/check_selftest/info.json
> @@ -0,0 +1,3 @@
> +{
> +  "run": ["check_selftest.sh"]
> +}

Re: [PATCH net 0/2] selftests: net: add missing tests to Makefile

[Hangbin Liu]

On Tue, May 17, 2022 at 12:45:17PM -0700, Jakub Kicinski wrote:
> Yes, we don't have the auto-reply. There's too much noise in some of
> the tests, but mostly it's because we don't want to encourage people
> posting patches just to build them. If it's a machine replying rather
> than a human some may think that it's okay. We already have
> jaw-droppingly expensive VM instance to keep up with the build volume.
> And the list is very busy. So we can't afford "post to run the CI"
> development model.

OK, I just afraid the developer doesn't check patchwork status.

> > +files=$(git show --name-status --oneline | grep -P '^A\ttools/testing/selftests/net/' | grep '\.sh$' | sed 's@A\ttools/testing/selftests/net/@@')
> > +for file in $files; do
> > +	if echo $file | grep forwarding; then
> > +		file=$(echo $file | sed 's/forwarding\///')
> > +		if ! grep -P "[\t| ]$file" tools/testing/selftests/net/forwarding/Makefile;then
> > +			echo "new test $file not in selftests/net/forwarding/Makefile" >&$DESC_FD
> > +			rc=1
> > +		fi
> > +	else
> > +		if ! grep -P "[\t| ]$file" tools/testing/selftests/net/Makefile;then
> > +			echo "new test $file not in selftests/net/Makefile" >&$DESC_FD
> > +			rc=1
> > +		fi
> 
> Does it matter which exact selftest makefile the changes are?

I only checked the tools/testing/selftests/net/Makefile and
tools/testing/selftests/net/forwarding/Makefile at present.
Maybe mptcp should also added?

> Maybe as a first stab we should just check if there are changes 
> to anything in tools/testing/selftests/.*/Makefile?

In my checking only shell scripts are checked, as most net net/forwarding tests
using shell script for testing. But other sub-component may use c binary or
python for testing. So I think there is no need to check all
tools/testing/selftests/.*/Makefile. WDYT?

Thanks
Hangbin

Re: [PATCH net 0/2] selftests: net: add missing tests to Makefile

[Jakub Kicinski]

On Wed, 18 May 2022 14:01:43 +0800 Hangbin Liu wrote:
> > > +files=$(git show --name-status --oneline | grep -P '^A\ttools/testing/selftests/net/' | grep '\.sh$' | sed 's@A\ttools/testing/selftests/net/@@')

FWIW this will list just the names of bash scripts with no decoration:

  git show --pretty="" --name-only -- tools/testing/selftests/*.sh

And we can get the names of the files with basename:

  for f in $(git show --pretty="" --name-only); do basename $f; done

> > > +for file in $files; do
> > > +	if echo $file | grep forwarding; then
> > > +		file=$(echo $file | sed 's/forwarding\///')
> > > +		if ! grep -P "[\t| ]$file" tools/testing/selftests/net/forwarding/Makefile;then
> > > +			echo "new test $file not in selftests/net/forwarding/Makefile" >&$DESC_FD
> > > +			rc=1
> > > +		fi
> > > +	else
> > > +		if ! grep -P "[\t| ]$file" tools/testing/selftests/net/Makefile;then
> > > +			echo "new test $file not in selftests/net/Makefile" >&$DESC_FD
> > > +			rc=1
> > > +		fi  
> > 
> > Does it matter which exact selftest makefile the changes are?  
> 
> I only checked the tools/testing/selftests/net/Makefile and
> tools/testing/selftests/net/forwarding/Makefile at present.
> Maybe mptcp should also added?

Right, mptcp is one example, then we also have
tools/testing/selftests/drivers/net/

There may be new directories added, then we'd need to keep updating 
the test.

> > Maybe as a first stab we should just check if there are changes 
> > to anything in tools/testing/selftests/.*/Makefile?  
> 
> In my checking only shell scripts are checked, as most net net/forwarding tests
> using shell script for testing. But other sub-component may use c binary or
> python for testing. So I think there is no need to check all
> tools/testing/selftests/.*/Makefile. WDYT?

Not sure I understand, let me explain what I meant in more detail. 
I think we should make it generic. For example check the Makefile 
in the same location as the script:

  grep $(basename $f) $(dirname $f)/Makefile

And maybe just to be safe one directory level down?

  grep $(basename $f) $(dirname $(dirname $f))/Makefile

Instead of hardcoding the expected paths.

Re: [PATCH net 0/2] selftests: net: add missing tests to Makefile

[Hangbin Liu]

On Wed, May 18, 2022 at 08:25:48AM -0700, Jakub Kicinski wrote:
> On Wed, 18 May 2022 14:01:43 +0800 Hangbin Liu wrote:
> > > > +files=$(git show --name-status --oneline | grep -P '^A\ttools/testing/selftests/net/' | grep '\.sh$' | sed 's@A\ttools/testing/selftests/net/@@')
> 
> FWIW this will list just the names of bash scripts with no decoration:
> 
>   git show --pretty="" --name-only -- tools/testing/selftests/*.sh
> 
> And we can get the names of the files with basename:
> 
>   for f in $(git show --pretty="" --name-only); do basename $f; done

This way is easier :)

> > python for testing. So I think there is no need to check all
> > tools/testing/selftests/.*/Makefile. WDYT?
> 
> Not sure I understand, let me explain what I meant in more detail. 
> I think we should make it generic. For example check the Makefile 
> in the same location as the script:
> 
>   grep $(basename $f) $(dirname $f)/Makefile
> 
> And maybe just to be safe one directory level down?
> 
>   grep $(basename $f) $(dirname $(dirname $f))/Makefile
> 
> Instead of hardcoding the expected paths.

Ah, got what you mean. Thanks. I will check how to update the script
and open PR after that.

Cheers
Hangbin