RFC patchtests: add initial bash completion tests

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---

Something is better than nothing.

 t/t9902-completion.sh |  198 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 198 insertions(+)
 create mode 100755 t/t9902-completion.sh

On Fri, Apr 06, 2012 at 10:28:39PM +0300, Felipe Contreras wrote:

Yes, but...

Won't this test break "make test" on systems without bash (or with bash elsewhere)?

I think you need to start with something like:

  #!/bin/sh

  if ! type bash; then
          echo '1..0 # SKIP skipping bash tests, bash not available'
          exit 0
  fi

  bash <<\END_OF_BASH_SCRIPT

  test_description='test bash completion'
  . ./test-lib.sh

  [... actual tests ...]

  test_done

  END_OF_BASH_SCRIPT

You could also run the main harness in the outer sh script, and just run bash inside each test, but I suspect that would end up cumbersome. I don't really care strongly which, as long as it gracefully skips on non-bash systems.

-Peff

Jeff King <peff@peff.net> writes:

;-)

This is a good example that sometimes something is worse than nothing, unless watched carefully by a competent reviewer.

Your suggestion makes sense to me.

On Fri, Apr 6, 2012 at 11:24 PM, Junio C Hamano <gitster@pobox.com> wrote:

And this is a good example of why you shouldn't blindly trust what a 'competent reviewer' says, as I'm pretty sure the comment is wrong.

But hey, if you prefer nothing, fine, drop this patch; let's continue to blindly modify the completion and fix regressions as they come. I guess I should drop my other tests as well.

I was planning to do a bit of reorganization to the bash completion after having some basic tests, but I'll just jump straight away to the actual patches then.

Cheers.

On Sat, Apr 07, 2012 at 12:28:05AM +0300, Felipe Contreras wrote:

Sorry, but I think you are wrong, and this is a perfect example of why you are sometimes frustrating to work with. Your patch is definitely a move in the right direction, and we would love to have something like it as part of git. And I'm sure it runs fine under _your_ setup. But the git community is much larger than just your setup, and your patch is a regression for other people, as it breaks "make test".

Did I say "let's throw away this patch"? No. I said "here is a problem with your patch", and I even sketched out a fix.

And nor do I think Junio was saying "let's throw it out". I think he was responding specifically to "something is better than nothing". It's _not_ better if it regresses other cases. So the patch as-is is not acceptable, but it could be made so.

But instead of taking the constructive criticism and iterating on your patch, you are ready to withdraw your patch. That seems silly when you have already done the hard part, and the fix to make the patch acceptable is the easy part.

But maybe I am wrong. Maybe there is no problem at all with your patch, and my analysis is wrong, and yours is right. I am willing to admit that as a possibility. But let's discuss it in the other part of the thread and find out, shall we?

-Peff

On Sat, Apr 7, 2012 at 12:42 AM, Jeff King <peff@peff.net> wrote:

My comment was to Junio, not to you; Junio said "sometimes something is worse than nothing"; if that's not preferring nothing, I don't know what it is.

I just sent an email where I assumed you preferred the status quo, sorry if I misinterpreted you, but my last patches to add some tests were never applied because you advocated the status quo; no tests.

Did you notice the "RFC" part of it? This patch was not meant to be applied as-is.

I have already done the "hard part" in the past and yet my patches were dropped because of commit message wording (or whatever), and we ended up with the status quo; no tests. So excuse me if I'm not eager to engage in a discussion in which you provide some "constructive criticism", Junio agrees, I disagree, and the patch never goes anywhere.

I didn't say there was no problem with the patch, I said the *particular* problem that you pointed out was incorrect (the shebang).

But OK, I will try once more to provide a more proper solution, let's see how it goes.

Cheers.

Felipe Contreras <felipe.contreras@gmail.com> writes:

We run the tests under whatever is configured as SHELL_PATH, so whatever you have on #! line does not matter much (except as a documentation).

But it would not make any sense to running the bash completion tests if the shell that is running the test script is *not* bash, would it? That is the point Peff made---the primary thing his message cared about is not to cause "make test" fail when your build does not use bash to run tests.

And that seems to have escaped you.

Adding a test for bash completion is a good thing, and blindly modifying the completion script without having good tests is a bad idea.

Just add the tests in the right way. Don't break tests for non-bash users.

On Fri, Apr 6, 2012 at 11:19 PM, Jeff King <peff@peff.net> wrote:

That is irrelevant, even if it's '#!/bin/foobar', it wouldn't make any difference since the actual command would be something like '$(SHELL_PATH) t9902-completion.sh'. Plus /bin/sh does not always point to bash, even when bash is available (see debian).

On Sat, Apr 07, 2012 at 12:21:35AM +0300, Felipe Contreras wrote:

True; I thought "prove" would run them directly, but even it uses $(SHELL_PATH) to run the tests. However, doesn't that mean your test will fail completely when $(SHELL_PATH) isn't bash?

So yes, the #! isn't relevant to "make test" (though marking it as #!/bin/sh does serve as documentation for what you expect, and does let people with a sane /bin/sh more easily run the test directly).

But my point still stands that you cannot assume that you are running bash, and you need to either find bash, or gracefully exit the test script if it is not available. Anything else will cause "make test" to fail on some systems (and indeed, applying and running your test, it breaks "make test" on my debian box with dash as /bin/sh).

Yes, that was my point, and why the example code I showed executed bash explicitly instead of relying on $(SHELL_PATH) to be set to it.

-Peff

On Sat, Apr 7, 2012 at 12:34 AM, Jeff King <peff@peff.net> wrote:

Yes.

No, it's the opposite; #!/bin/sh would imply that you can run this with any POSIX shell; you can't. 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), but that would not affect 'make tests', 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'.

So? 'make test' fails on my Arch Linux box which doesn't have /usr/bin/python, which is presumably why there is NO_PYTHON. 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).

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

Felipe Contreras <felipe.contreras@gmail.com> writes:

If you "git grep NO_PYTHON", you would notice that t/test-lib.sh does have a provision to set PYTHON prerequisite, so fixing it presumably is just the matter of marking appropriate tests with the prerequisite, no? Which ones are broken for you? Complaining about it in a thread that does not directly related to that "on a box without python some tests are broken" issue is a very good way to leave it unfixed.

"The box does not have bash" and "The builder does not wish to let the scripts run with bash" are two separate things. SHELL_PATH is often set to /bin/dash even on a box that has /bin/bash because it is much faster to run scripted Porcelains with, but the end user of the resulting build may still want to use bash in her interactive session.

For this "the rest of the script is all meant to test stuff written for bash" test, I think the right approach is to explicitly detect the presense of bash (i.e. "Can the end user run bash for her interactive session? Otherwise we cannot test and there is no point testing"), and feed these tests explicitly to bash. Perhaps by putting your bash specific test in t/t9902/bash-completion-test.bash and calling it from the t9902-completionl.sh script that is meant to be run with $SHELL_PATH, like this:

== t/t9902-completion.sh == #!/bin/sh

. ./test-lib.sh

if bash is available ;# do *not* check if we are running under bash!!
then
	bash "$TEST_DIRECTORY/t9902/bash-comletion-test.bash"
fi

if zsh is available ;# do *not* check if we are running under zsh!!
then
	zsh "$TEST_DIRECTORY/t9902/zsh-comletion-test.zsh"
fi

test_done

On Sat, Apr 7, 2012 at 1:46 AM, Junio C Hamano <gitster@pobox.com> wrote:

Yes, but you have to specify NO_PYTHON, otherwise it would just assume python is installed.

You would still have to specify NO_PYTHON.

In fact, I do have python, but the program name is python2, but if I do this:

 export PYTHON_PATH=/usr/bin/python2

The tests fail on remote-helpers.

My point is that the argument "this is worst than nothing because tests would break on X systems" seems rather weak to me considering that tests are already broken on Y systems.

So? SHELL_PATH would not interfere with NO_BASH/NO_ZSH. But in any case, I've sent a patch that detects bash presence, similarly to Jeff's patch.

Cheers.

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

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).

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.

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).

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"?

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.

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".

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

On Fri, Apr 6, 2012 at 11:19 PM, Jeff King <peff@peff.net> wrote:

What about this instead?

--- #!/bin/sh

if type bash >/dev/null 2>&1
then
	# execute inside bash
	[ -z "$BASH" ] && exec bash $0
else
	echo '1..0 #SKIP skipping bash completion tests; bash not available'
	exit 0
fi
---

Felipe Contreras <felipe.contreras@gmail.com> writes:

Please follow the CodingGuidelines and use

	test -z "$BASH" && ...

instead.

Other than that, it is very much simple and straight-forward.  I like it.

Junio C Hamano <gitster@pobox.com> writes:

It probably is even simpler to do it like this.

if test -z "$BASH" && ! exec bash "$0" "$@"
then
	echo '1..0 #SKIP...'
        exit 0
fi

... rest of the test starting with ". ./test-lib.sh" ...

On Sat, Apr 7, 2012 at 2:33 AM, Junio C Hamano <gitster@pobox.com> wrote:

That would exit immediately with an error:

t9902-completion.sh: 6: exec: bash: not found make: *** [t9902-completion.sh] Error 127

On Sat, Apr 07, 2012 at 02:12:22AM +0300, Felipe Contreras wrote:

Yes, that's fine. It should be functionally equivalent to what I posted, but without having to invoke an extra bash when we are already running it (and avoiding the giant here-doc, though that is simply ugly and not a functional problem).

One minor fix. I think it should be:

  exec bash $0 "$@"

to pass arguments (e.g., "-v -i") to the bash sub-process.

-Peff