Re: [Quilt-dev] [patch] Make it possible to test the bash completion script

[Jean Delvare]

Hi Martin,

Le Wednesday 26 February 2014 à 09:20 +0100, address@hidden a
écrit :
> pièce jointe document texte brut (test-completion)
> - Add a helper script that runs a completion and checks that it
>   reaches the expected outcome
> - Add an example of test (in our usual test suite) using it
> - Caveat: you probably need to chmod +x test/run-test-completion.in

No, you don't. run-test-completion must be executable, not
run-test-completion.in, just like all other scripts.

> This closes #41688.
> 
> ---
>  Makefile.in                 |    6 +++++-
>  test/completion.test        |   10 ++++++++++
>  test/run-test-completion.in |   42 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 57 insertions(+), 1 deletion(-)
> 
> Index: b/test/completion.test
> ===================================================================
> --- /dev/null
> +++ b/test/completion.test
> @@ -0,0 +1,10 @@
> +$ ../run-test-completion add quilt ad
> +>~ PASS:.*

I don't like this format. The test suite runner can already compare the
actual output with the expected output. Delegating this to
run-test-completion here is inconsistent.

Also the relative path to call run-test-completion is kind of fragile.
For everything else, we make sure the proper directory is added in front
of the PATH first, it would be nice if we could do the same here.

> +
> +$ ../run-test-completion "add annotate applied" quilt a
> +>~ PASS:.*
> +
> +$ ../run-test-completion "" quilt toppp
> +>~ PASS:.*
> +
> +

Please avoid blank lines at end of files.

> Index: b/test/run-test-completion.in
> ===================================================================
> --- /dev/null
> +++ b/test/run-test-completion.in
> @@ -0,0 +1,42 @@
> +#! @BASH@
> +
> +# Little test runner for the bash completion
> +
> +# Usage:   run-test-completion "expected output" the command line to complete

I would expect:
Usage: run-test-completion "the command line to complete"
Output: proposed completions

> +#
> +# $ run-test-completion "add" quilt ad
> +# PASS: quilt ad -> add
> +# $ run-test-completion "add" quilt a
> +# FAIL: quilt a -> add annotate applied  !=  add
> +# $ run-test-completion "" quilt toppp
> +# PASS: quilt toppp -> 
> +
> +
> +#  This script is free software; you can redistribute it and/or modify
> +#  it under the terms of the GNU General Public License version 2 as
> +#  published by the Free Software Foundation.
> +#
> +#  See the COPYING and AUTHORS files for more details.
> +
> +
> +set -e
> +source ../../bash_completion

This relative path is fragile...

> +expected_output=$1
> +shift
> +
> +declare -a COMP_WORDS=()
> +for arg in "$@" ; do
> +  address@hidden
> +done

I think you can do COMP_WORDS=("$@").

> address@hidden

This will assign for example "2-1" to COMP_CWORD, not "1". Seems weird
that it works, but... it does. I guess it is evaluated later on.

> +
> +set +e # _quilt_completion fails when completing something with no existing 
> completion, such as "quilt topppp"

I don't think it was worth setting -e in the first place. It can cause
more trouble than in helps anyway.

> +_quilt_completion -o filenames quilt

What's the effect of -o filenames here? Function _quilt_completion
doesn't take parameters. I'm afraid this means that we won't be able to
test all features of the completion script. We can test the completion
of commands, options and patch names, but not completion of file and
directory names?

> +observed_output="${COMPREPLY[*]}"
> +
> +if [ "$expected_output" = "$observed_output" ] ; then
> +  echo "PASS: $@ -> $expected_output"
> +else
> +  echo "FAIL: $@ -> $observed_output  !=  $expected_output"
> +fi

As said before I'd prefer a simple "echo ${COMPREPLY[*]}" and let the
caller do the comparison.

> Index: b/Makefile.in
> ===================================================================
> --- a/Makefile.in
> +++ b/Makefile.in
> @@ -79,6 +79,9 @@
>  SRC +=               $(QUILT_SRC:%=quilt/%)
>  DIRT +=              $(QUILT_IN:%=quilt/%)
>  
> +SRC +=               test/run-test-completion.in
> +DIRT +=              test/run-test-completion
> +

Please move this close to the rest of the SRC and DIRT settings related
to files under test/.

>  SCRIPTS_IN :=        patchfns inspect dependency-graph edmail        \
>               remove-trailing-ws backup-files
>  
> @@ -148,7 +151,8 @@
>  
>  scripts : $(BIN:%=bin/%) $(QUILT:%=quilt/%)                          \
>         $(SCRIPTS:%=quilt/scripts/%)                                  \
> -       $(if $(PATCH_WRAPPER),bin/patch-wrapper)
> +       $(if $(PATCH_WRAPPER),bin/patch-wrapper)                      \
> +       test/run-test-completion
>  
>  dist : clean $(PACKAGE)-$(VERSION).tar.gz
>  


-- 
Jean Delvare
SUSE L3 Support