Re: [PATCH nft] tests: shell: Drop redefinition of DIFF variable

[Phil Sutter <phil@nwl.cc>]

Hi,

On Mon, Jun 15, 2020 at 02:40:55PM +0200, Stefano Brivio wrote:
> On Mon, 15 Jun 2020 13:54:24 +0200
> Phil Sutter <phil@nwl.cc> wrote:
> 
> > Hi Stefano,
> > 
> > On Mon, Jun 15, 2020 at 12:18:11PM +0200, Stefano Brivio wrote:
> > > On Mon, 15 Jun 2020 11:00:44 +0200
> > > Phil Sutter <phil@nwl.cc> wrote:
> > >   
> > > > Hi Stefano,
> > > > 
> > > > On Sun, Jun 14, 2020 at 11:41:49PM +0200, Stefano Brivio wrote:  
> > > > > Commit 7d93e2c2fbc7 ("tests: shell: autogenerate dump verification")
> > > > > introduced the definition of DIFF at the top of run-tests.sh, to make
> > > > > it visually part of the configuration section. Commit 68310ba0f9c2
> > > > > ("tests: shell: Search diff tool once and for all") override this
> > > > > definition.
> > > > > 
> > > > > Drop the unexpected redefinition of DIFF.    
> > > > 
> > > > I would fix it the other way round, dropping the first definition.  
> > > 
> > > Then it's not visibly configurable anymore. It was in 2018, so it
> > > looks like a regression to me.  
> > 
> > Hmm? Commit 68310ba0f9c2 is from January this year?!
> 
> Commit 7d93e2c2fbc7 (which makes it "configurable") is from March 2018.

I think you're misinterpreting that commit regarding an attempt at
making diff binary configurable.

> > > > It's likely a missing bit from commit 68310ba0f9c20, the second
> > > > definition is in line with FIND and MODPROBE definitions immediately
> > > > preceding it.  
> > > 
> > > I see a few issues with those blocks:
> > > 
> > > - that should be a single function called (once or multiple times) for
> > >   nft, find, modprobe, diff, anything else we'll need in the future.
> > >   It would avoid any oversight of this kind and keep the script
> > >   cleaner. For example, what makes sort(1) special here?  
> > 
> > It is simply grown and therefore a bit inconsistent.
> 
> Yes, hence worth a minor rework perhaps? I can take care of this at
> some point.

Sure, why not. For now it works just fine, apart from the duplicate
line which is merely a cosmetic issue.

> > > - quotes are applied inconsistently. If you expect multiple words from
> > >   which(1), then variables should also be quoted when used, otherwise
> > >   the check might go through, and we fail later  
> > 
> > There are needless quotes around $(...) but within single brackets we
> > need them if we expect empty or multi-word strings. Typical output would
> > be 'diff not found' and using '[ ! -x "$DIFF" ]' we check if which
> > returned diff's path or said error message. We don't expect diff's path
> > to contain spaces, hence no quoting afterwards.
> 
> Probably stupid but:
> 
> [ break nft ]
> 
> # mkdir "my secret binaries"
> # cp /usr/bin/diff "my secret binaries/"
> # export PATH="my secret binaries:$PATH"
> # nftables/tests/shell/run-tests.sh nftables/tests/shell/testcases/listing/0003table_0
> I: using nft command: nftables/tests/shell/../../src/nft

Yes, I mentioned we don't expect $PATH contents to contain spaces.
Having such a thing would probably break more things than just
nftables testsuite. :)

> W: [FAILED]	nftables/tests/shell/testcases/listing/0003table_0: got 127
> nftables/tests/shell/testcases/listing/0003table_0: line 15: my: command not found
> 
> I: results: [OK] 0 [FAILED] 1 [TOTAL] 1
> 
> or, perhaps more reasonable:
> 
> # grep DIFF=\" nftables/tests/shell/run-tests.sh
> DIFF="diff -y"

This is no guaranteed functionality. There's no comment or anything
stating you could change the DIFF definition atop the script to
customize diff behaviour. So strictly speaking you're on your own if you
do that and a broken testsuite is not unexpected.

> # nftables/tests/shell/run-tests.sh nftables/tests/shell/testcases/listing/0003table_0
> I: using nft command: nftables/tests/shell/../../src/nft
> 
> W: [FAILED]	nftables/tests/shell/testcases/listing/0003table_0: got 1
> 
> I: results: [OK] 0 [FAILED] 1 [TOTAL] 1
> 
> Personally, this bothers me more than some cheap, extra quotes. If the
> general agreement is that it's not worth to add quotes to fix this,
> fine, I would skip this the day I have time to propose the single
> checking function rework I mentioned. Just let me know.

As said, the quotes are there to cover the expected 'which' output, no
more and no less. Supporting user-defined diff-command (or custom
options) is new functionality IMO. I'm totally fine with that and merely
want to point out we're not talking about fixing a bug here.

Cheers, Phil