| Message ID | 20230801114236.27235-1-phil@nwl.cc |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [nft] tests: shell: Review test-cases for destroy command | expand |
Hi, Thanks Phil, LGTM. Just one comment: On 01/08/2023 13:42, Phil Sutter wrote: > Having separate files for successful destroy of existing and > non-existing objects is a bit too much, just combine them into one. > > While being at it: > > * Check that deleted objects are actually gone I think that is done automatically with the expected ruleset.. isn't? If not, what is the purpose of having them? Thanks, Fernando. > * No bashisms, using /bin/sh is fine > * Append '-e' to shebang itself instead of calling 'set' > * Use 'nft -a -e' instead of assuming the created rule's handle value > * Shellcheck warned about curly braces, quote them > > Signed-off-by: Phil Sutter <phil@nwl.cc> > --- > tests/shell/testcases/chains/0044chain_destroy_0 | 12 ++++++++---- > tests/shell/testcases/chains/0045chain_destroy_0 | 8 -------- > tests/shell/testcases/flowtable/0015destroy_0 | 10 +++++++--- > tests/shell/testcases/flowtable/0016destroy_0 | 6 ------ > tests/shell/testcases/maps/0014destroy_0 | 11 +++++++---- > tests/shell/testcases/maps/0015destroy_0 | 7 ------- > tests/shell/testcases/rule_management/0012destroy_0 | 10 ++++++++-- > tests/shell/testcases/rule_management/0013destroy_0 | 8 -------- > tests/shell/testcases/sets/0072destroy_0 | 11 +++++++---- > tests/shell/testcases/sets/0073destroy_0 | 7 ------- > 10 files changed, 37 insertions(+), 53 deletions(-) > delete mode 100755 tests/shell/testcases/chains/0045chain_destroy_0 > delete mode 100755 tests/shell/testcases/flowtable/0016destroy_0 > delete mode 100755 tests/shell/testcases/maps/0015destroy_0 > delete mode 100755 tests/shell/testcases/rule_management/0013destroy_0 > delete mode 100755 tests/shell/testcases/sets/0073destroy_0 > > diff --git a/tests/shell/testcases/chains/0044chain_destroy_0 b/tests/shell/testcases/chains/0044chain_destroy_0 > index 070021cf12f24..fbc1044474b12 100755 > --- a/tests/shell/testcases/chains/0044chain_destroy_0 > +++ b/tests/shell/testcases/chains/0044chain_destroy_0 > @@ -1,7 +1,11 @@ > -#!/bin/bash > - > -set -e > +#!/bin/sh -e > > $NFT add table t > > -$NFT destroy chain t nochain > +# pass for non-existent chain > +$NFT destroy chain t c > + > +# successfully delete existing chain > +$NFT add chain t c > +$NFT destroy chain t c > +! $NFT list chain t c 2>/dev/null > diff --git a/tests/shell/testcases/chains/0045chain_destroy_0 b/tests/shell/testcases/chains/0045chain_destroy_0 > deleted file mode 100755 > index b356f8f885ca4..0000000000000 > --- a/tests/shell/testcases/chains/0045chain_destroy_0 > +++ /dev/null > @@ -1,8 +0,0 @@ > -#!/bin/bash > - > -set -e > - > -$NFT add table t > -$NFT add chain t c > - > -$NFT destroy chain t c > diff --git a/tests/shell/testcases/flowtable/0015destroy_0 b/tests/shell/testcases/flowtable/0015destroy_0 > index 4828d8187018c..01af84869a3e0 100755 > --- a/tests/shell/testcases/flowtable/0015destroy_0 > +++ b/tests/shell/testcases/flowtable/0015destroy_0 > @@ -1,7 +1,11 @@ > -#!/bin/bash > +#!/bin/sh -e > > -set -e > $NFT add table t > -$NFT add flowtable t f { hook ingress priority 10 \; devices = { lo }\; } > > +# pass for non-existent flowtable > $NFT destroy flowtable t f > + > +# successfully delete existing flowtable > +$NFT add flowtable t f '{ hook ingress priority 10; devices = { lo }; }' > +$NFT destroy flowtable t f > +! $NFT list flowtable t f 2>/dev/null > diff --git a/tests/shell/testcases/flowtable/0016destroy_0 b/tests/shell/testcases/flowtable/0016destroy_0 > deleted file mode 100755 > index ce23c753365af..0000000000000 > --- a/tests/shell/testcases/flowtable/0016destroy_0 > +++ /dev/null > @@ -1,6 +0,0 @@ > -#!/bin/bash > - > -set -e > -$NFT add table t > - > -$NFT destroy flowtable t f > diff --git a/tests/shell/testcases/maps/0014destroy_0 b/tests/shell/testcases/maps/0014destroy_0 > index b769276d52599..6e639a11e2ac2 100755 > --- a/tests/shell/testcases/maps/0014destroy_0 > +++ b/tests/shell/testcases/maps/0014destroy_0 > @@ -1,8 +1,11 @@ > -#!/bin/bash > - > -set -e > +#!/bin/sh -e > > $NFT add table x > -$NFT add map x y { type ipv4_addr : ipv4_addr\; } > > +# pass for non-existent map > +$NFT destroy map x y > + > +# successfully delete existing map > +$NFT add map x y '{ type ipv4_addr : ipv4_addr; }' > $NFT destroy map x y > +! $NFT list map x y 2>/dev/null > diff --git a/tests/shell/testcases/maps/0015destroy_0 b/tests/shell/testcases/maps/0015destroy_0 > deleted file mode 100755 > index abad4d57e7221..0000000000000 > --- a/tests/shell/testcases/maps/0015destroy_0 > +++ /dev/null > @@ -1,7 +0,0 @@ > -#!/bin/bash > - > -set -e > - > -$NFT add table x > - > -$NFT destroy map x nonmap > diff --git a/tests/shell/testcases/rule_management/0012destroy_0 b/tests/shell/testcases/rule_management/0012destroy_0 > index 1b61155e9b7ef..57c4da99c285c 100755 > --- a/tests/shell/testcases/rule_management/0012destroy_0 > +++ b/tests/shell/testcases/rule_management/0012destroy_0 > @@ -1,7 +1,13 @@ > -#!/bin/bash > +#!/bin/sh -e > > -set -e > $NFT add table t > $NFT add chain t c > > +# pass for non-existent rule > $NFT destroy rule t c handle 3333 > + > +# successfully delete existing rule > +handle=$($NFT -a -e insert rule t c accept | \ > + sed -n 's/.*handle \([0-9]*\)$/\1/p') > +$NFT destroy rule t c handle "$handle" > +! $NFT list chain t c | grep -q accept > diff --git a/tests/shell/testcases/rule_management/0013destroy_0 b/tests/shell/testcases/rule_management/0013destroy_0 > deleted file mode 100755 > index 895c24a4dacba..0000000000000 > --- a/tests/shell/testcases/rule_management/0013destroy_0 > +++ /dev/null > @@ -1,8 +0,0 @@ > -#!/bin/bash > - > -set -e > -$NFT add table t > -$NFT add chain t c > -$NFT insert rule t c accept # should have handle 2 > - > -$NFT destroy rule t c handle 2 > diff --git a/tests/shell/testcases/sets/0072destroy_0 b/tests/shell/testcases/sets/0072destroy_0 > index c9cf9ff716349..80d24b4760099 100755 > --- a/tests/shell/testcases/sets/0072destroy_0 > +++ b/tests/shell/testcases/sets/0072destroy_0 > @@ -1,8 +1,11 @@ > -#!/bin/bash > - > -set -e > +#!/bin/sh -e > > $NFT add table x > -$NFT add set x s {type ipv4_addr\; size 2\;} > > +# pass for non-existent set > +$NFT destroy set x s > + > +# successfully delete existing set > +$NFT add set x s '{type ipv4_addr; size 2;}' > $NFT destroy set x s > +! $NFT list set x s 2>/dev/null > diff --git a/tests/shell/testcases/sets/0073destroy_0 b/tests/shell/testcases/sets/0073destroy_0 > deleted file mode 100755 > index a9d65a5541c53..0000000000000 > --- a/tests/shell/testcases/sets/0073destroy_0 > +++ /dev/null > @@ -1,7 +0,0 @@ > -#!/bin/bash > - > -set -e > - > -$NFT add table x > - > -$NFT destroy set x s
On Thu, Aug 03, 2023 at 12:45:14AM +0200, Fernando F. Mancera wrote: > Thanks Phil, LGTM. Just one comment: > > On 01/08/2023 13:42, Phil Sutter wrote: > > Having separate files for successful destroy of existing and > > non-existing objects is a bit too much, just combine them into one. > > > > While being at it: > > > > * Check that deleted objects are actually gone > > I think that is done automatically with the expected ruleset.. isn't? If > not, what is the purpose of having them? Oh, I missed those entirely! I'll prepare a v2 dropping the manual checks and deleting the leftovers. Thanks, Phil
diff --git a/tests/shell/testcases/chains/0044chain_destroy_0 b/tests/shell/testcases/chains/0044chain_destroy_0 index 070021cf12f24..fbc1044474b12 100755 --- a/tests/shell/testcases/chains/0044chain_destroy_0 +++ b/tests/shell/testcases/chains/0044chain_destroy_0 @@ -1,7 +1,11 @@ -#!/bin/bash - -set -e +#!/bin/sh -e $NFT add table t -$NFT destroy chain t nochain +# pass for non-existent chain +$NFT destroy chain t c + +# successfully delete existing chain +$NFT add chain t c +$NFT destroy chain t c +! $NFT list chain t c 2>/dev/null diff --git a/tests/shell/testcases/chains/0045chain_destroy_0 b/tests/shell/testcases/chains/0045chain_destroy_0 deleted file mode 100755 index b356f8f885ca4..0000000000000 --- a/tests/shell/testcases/chains/0045chain_destroy_0 +++ /dev/null @@ -1,8 +0,0 @@ -#!/bin/bash - -set -e - -$NFT add table t -$NFT add chain t c - -$NFT destroy chain t c diff --git a/tests/shell/testcases/flowtable/0015destroy_0 b/tests/shell/testcases/flowtable/0015destroy_0 index 4828d8187018c..01af84869a3e0 100755 --- a/tests/shell/testcases/flowtable/0015destroy_0 +++ b/tests/shell/testcases/flowtable/0015destroy_0 @@ -1,7 +1,11 @@ -#!/bin/bash +#!/bin/sh -e -set -e $NFT add table t -$NFT add flowtable t f { hook ingress priority 10 \; devices = { lo }\; } +# pass for non-existent flowtable $NFT destroy flowtable t f + +# successfully delete existing flowtable +$NFT add flowtable t f '{ hook ingress priority 10; devices = { lo }; }' +$NFT destroy flowtable t f +! $NFT list flowtable t f 2>/dev/null diff --git a/tests/shell/testcases/flowtable/0016destroy_0 b/tests/shell/testcases/flowtable/0016destroy_0 deleted file mode 100755 index ce23c753365af..0000000000000 --- a/tests/shell/testcases/flowtable/0016destroy_0 +++ /dev/null @@ -1,6 +0,0 @@ -#!/bin/bash - -set -e -$NFT add table t - -$NFT destroy flowtable t f diff --git a/tests/shell/testcases/maps/0014destroy_0 b/tests/shell/testcases/maps/0014destroy_0 index b769276d52599..6e639a11e2ac2 100755 --- a/tests/shell/testcases/maps/0014destroy_0 +++ b/tests/shell/testcases/maps/0014destroy_0 @@ -1,8 +1,11 @@ -#!/bin/bash - -set -e +#!/bin/sh -e $NFT add table x -$NFT add map x y { type ipv4_addr : ipv4_addr\; } +# pass for non-existent map +$NFT destroy map x y + +# successfully delete existing map +$NFT add map x y '{ type ipv4_addr : ipv4_addr; }' $NFT destroy map x y +! $NFT list map x y 2>/dev/null diff --git a/tests/shell/testcases/maps/0015destroy_0 b/tests/shell/testcases/maps/0015destroy_0 deleted file mode 100755 index abad4d57e7221..0000000000000 --- a/tests/shell/testcases/maps/0015destroy_0 +++ /dev/null @@ -1,7 +0,0 @@ -#!/bin/bash - -set -e - -$NFT add table x - -$NFT destroy map x nonmap diff --git a/tests/shell/testcases/rule_management/0012destroy_0 b/tests/shell/testcases/rule_management/0012destroy_0 index 1b61155e9b7ef..57c4da99c285c 100755 --- a/tests/shell/testcases/rule_management/0012destroy_0 +++ b/tests/shell/testcases/rule_management/0012destroy_0 @@ -1,7 +1,13 @@ -#!/bin/bash +#!/bin/sh -e -set -e $NFT add table t $NFT add chain t c +# pass for non-existent rule $NFT destroy rule t c handle 3333 + +# successfully delete existing rule +handle=$($NFT -a -e insert rule t c accept | \ + sed -n 's/.*handle \([0-9]*\)$/\1/p') +$NFT destroy rule t c handle "$handle" +! $NFT list chain t c | grep -q accept diff --git a/tests/shell/testcases/rule_management/0013destroy_0 b/tests/shell/testcases/rule_management/0013destroy_0 deleted file mode 100755 index 895c24a4dacba..0000000000000 --- a/tests/shell/testcases/rule_management/0013destroy_0 +++ /dev/null @@ -1,8 +0,0 @@ -#!/bin/bash - -set -e -$NFT add table t -$NFT add chain t c -$NFT insert rule t c accept # should have handle 2 - -$NFT destroy rule t c handle 2 diff --git a/tests/shell/testcases/sets/0072destroy_0 b/tests/shell/testcases/sets/0072destroy_0 index c9cf9ff716349..80d24b4760099 100755 --- a/tests/shell/testcases/sets/0072destroy_0 +++ b/tests/shell/testcases/sets/0072destroy_0 @@ -1,8 +1,11 @@ -#!/bin/bash - -set -e +#!/bin/sh -e $NFT add table x -$NFT add set x s {type ipv4_addr\; size 2\;} +# pass for non-existent set +$NFT destroy set x s + +# successfully delete existing set +$NFT add set x s '{type ipv4_addr; size 2;}' $NFT destroy set x s +! $NFT list set x s 2>/dev/null diff --git a/tests/shell/testcases/sets/0073destroy_0 b/tests/shell/testcases/sets/0073destroy_0 deleted file mode 100755 index a9d65a5541c53..0000000000000 --- a/tests/shell/testcases/sets/0073destroy_0 +++ /dev/null @@ -1,7 +0,0 @@ -#!/bin/bash - -set -e - -$NFT add table x - -$NFT destroy set x s
Having separate files for successful destroy of existing and non-existing objects is a bit too much, just combine them into one. While being at it: * Check that deleted objects are actually gone * No bashisms, using /bin/sh is fine * Append '-e' to shebang itself instead of calling 'set' * Use 'nft -a -e' instead of assuming the created rule's handle value * Shellcheck warned about curly braces, quote them Signed-off-by: Phil Sutter <phil@nwl.cc> --- tests/shell/testcases/chains/0044chain_destroy_0 | 12 ++++++++---- tests/shell/testcases/chains/0045chain_destroy_0 | 8 -------- tests/shell/testcases/flowtable/0015destroy_0 | 10 +++++++--- tests/shell/testcases/flowtable/0016destroy_0 | 6 ------ tests/shell/testcases/maps/0014destroy_0 | 11 +++++++---- tests/shell/testcases/maps/0015destroy_0 | 7 ------- tests/shell/testcases/rule_management/0012destroy_0 | 10 ++++++++-- tests/shell/testcases/rule_management/0013destroy_0 | 8 -------- tests/shell/testcases/sets/0072destroy_0 | 11 +++++++---- tests/shell/testcases/sets/0073destroy_0 | 7 ------- 10 files changed, 37 insertions(+), 53 deletions(-) delete mode 100755 tests/shell/testcases/chains/0045chain_destroy_0 delete mode 100755 tests/shell/testcases/flowtable/0016destroy_0 delete mode 100755 tests/shell/testcases/maps/0015destroy_0 delete mode 100755 tests/shell/testcases/rule_management/0013destroy_0 delete mode 100755 tests/shell/testcases/sets/0073destroy_0