Re: [RFC/PATCH] tests: add initial bash completion tests

[Jeff King <peff@peff.net>]

On Sat, Apr 07, 2012 at 01:05:51AM +0300, Felipe Contreras wrote:

> > So yes, the #! isn't relevant to "make test" (though marking it as
> > #!/bin/sh does serve as documentation for what you expect,
> 
> No, it's the opposite; #!/bin/sh would imply that you can run this
> with any POSIX shell; you can't.

I think you are conflating the test _harness_ with the contents of the
tests themselves.  The harness must run in a POSIX shell, because it
needs to run everywhere and at least say "sorry, I must skip these tests
because I don't have bash".

This issue is made more confusing by the fact that the tool you are
testing is itself a shell. But let us imagine for a moment that you want
to test some tool "foo" that may or may not exist on the system. Then
you would execute the harness via the POSIX shell. Then you would first
check whether "foo" is available (either using "type", or by checking a
build option like NO_FOO). If not, then you would skip all tests, and if
so, you would continue to perform tests using "foo".

So we want the same effect here; you must be able to run the bare
minimum of harness to get to the "skip or run" decision with a POSIX
shell. So you could write it as a pure-sh harness that runs:

  bash -c '. git-completion.bash &&
           some_actual_test'

in each test (or some equivalent). But because the tool you are testing
is in fact a POSIX-compatible shell, you have chosen to instead run all
of the post-skip-decision parts directly inside the harness. Which is
fine by me, but I think is what makes the issue more confusing.

So what is the appropriate shebang line? The first part _must_ be POSIX
shell, but the latter part need not be. I would argue for the lowest
common denominator, as that is what you need for the script to get to
the decision point (and possibly not just exit, but actually run bash).

> For example, if /bin/sh points to
> dash (which is the case in debian AFAIK), the test would fail in the
> middle of it. #!/bin/bash would be more proper; it would properly
> document that you need bash to run this. Sure, maybe bash is not in
> /bin (I have never seen that),

You see this on systems where bash is not part of the base system (e.g.,
Solaris). I think it is also the case on FreeBSD (because bash comes
from ports and gets installed in /usr/local), though it has been years
since I have run FreeBSD, so that may no longer be the case.

> but that would not affect 'make tests',

The shebang line does not affect people who run "make test", but the
fact that the script does not run under a POSIX shell does (and that is
primarily what I was complaining about).

> only the people that want to run this directly, which I suspect are
> very, *very* few, and they would immediately see a clear error:
> './blah: /bin/bash: bad interpreter: No such file or directory', and
> then they could easily try 'bash ./blah'.

Yes, but why make them do that? The script must be able to run and at
least exit gracefully under a POSIX shell (or hopefully find and exec
bash itself) in order for "make test" to work. So since it must contain
code to handle this already, why not have #!/bin/sh in the shebang line
and simply let the code run as it would under "make test"?

> So? 'make test' fails on my Arch Linux box which doesn't have
> /usr/bin/python, which is presumably why there is NO_PYTHON.

We design our test suite to succeed everywhere, and Junio does not push
out master unless it passes (or skips) all tests. If it doesn't, then
either there is a build option (like NO_PYTHON) that you should set, or
there is a bug which should be fixed.  But that is not an excuse to
blatantly violate the existing property of the suite with a new test.

> Instead of doing some nasty hacks such as 'bash
> <<\END_OF_BASH_SCRIPT', it would be much easier to have a NO_BASH
> option. And in fact, when zsh completion tests are available, NO_ZSH
> (probably on by default).

Yeah, we could do that. Though I think it is nice to run the bash tests
even when $SHELL_PATH does not point to bash, since it gets test
coverage on systems that would otherwise not have it (e.g., on debian
systems where /bin/sh is dash). So I would prefer to find and run bash
if it exists, and save NO_BASH for the case of "yes, I have bash, but it
is old or broken, so do not bother to run the tests".

> But you clearly prefer the status quo, so I'm not going to bother.

I'm not even sure what to make of this. I've already said I like the
concept of your patch, that it does not meet the requirements of the
test suite, and tried to work out a solution for meeting that
requirement so we can apply it. How in the world does that equate to the
status quo?

-Peff