| Message ID | 20220428044511.227416-1-liuhangbin@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | selftests: net: add missing tests to Makefile | expand |
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.
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!
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"] +}
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"] > +}
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
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.
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