Re: [PATCH RFC] tests: add feature probing

[Florian Westphal <fw@strlen.de>]

Thomas Haller <thaller@redhat.com> wrote:
> > +check_features()
> > +{
> > +       for ffilename in features/*.nft; do
> > +               feature=${ffilename#*/}
> > +               feature=${feature%*.nft}
> > +               eval NFT_HAVE_${feature}=0
> > +               $NFT -f "$ffilename" 2>/dev/null
> 
> is the "--check" option here missing? At least, the commit message says

Yes, added.

> I think it should run in a separate netns too.

Should not make a difference due to --check, nothing
is altered.

> OK, that's nice, to see in the output.
> 
> But why this "nft -f" specific detection? Why not just executable
> scripts?

Because I want it to be simple, it will be enough to drop a ruleset
with the feature into features/ durectory and run-tests.sh would do the
reset.

While its possible to launch nft as a script via

 #!/usr/bin/env nft

This would use the system nft, not the newly built one.  And fudging
with $PATH seems ugly...

> Also, why should "run-tests.sh" pre-evaluate those NFT_HAVE_* features?
> Just let each tests:
> 
>      if ! "$BASEDIR/features/chain-binding" ; then
>          echo " defaultchain"
>          return
>      fi
> 
> then the checks are more flexible (arbitrary executables).

I could do that, but I don't see the need for arbitrary scripts so far.

> Downside, if the check is time consuming (which it shouldn't),

Agree, it should not.

> then
> tests might call it over and over. Workaround for that: have "run-
> tests.sh" prepare a temporary directory and export it as
> $NFT_TEST_TMPDIR". Then "features/chain-binding" can cache the result
> there. "run-tests.sh" would delete the directory afterwards.
> 
> If you want to print those features, you can still let run-tests.sh
> iterate over "$BASEDIR"/features/* and print the result.

Yes, thats a working alternative but I don't see the need for this.

> > diff --git a/tests/shell/testcases/transactions/30s-stress
> > b/tests/shell/testcases/transactions/30s-stress
> > index 4d3317e22b0c..924e7e28f97e 100755
> > --- a/tests/shell/testcases/transactions/30s-stress
> > +++ b/tests/shell/testcases/transactions/30s-stress
> > @@ -16,6 +16,18 @@ if [ x = x"$NFT" ] ; then
> >         NFT=nft
> >  fi
> >  
> > +if [ -z $NFT_HAVE_chain_binding ];then
> > +       NFT_HAVE_chain_binding=0
> > +       mydir=$(dirname $0)
> 
> I think run-tests.sh should export the base directory, like "$BASEDIR",
> or "$NFT_TEST_BASEDIR". Tests should use it (and rely to have it).

Can do that.

> ah, you'd have each tests re-implement the check? So that they can run
> without the "run-tests.sh" wrapper?

No, just 30s-stress.  This test is special because its random by nature
and it makes sense to run it for multiple hours once in a while.

> The point here is, that if the "run-tests.sh" wrapper can provide
> something useful, then tests should be able to rely on it (and don't
> implement a fallback path).

Yes, no other script will do this.
I'll send an updated batch soon to see how this looks like with more
feature tests in place.