Re: [PATCH v3 1/9] t5520: fixup file contents comparisons

From: Paul Tan <pyokagan@gmail.com>
Date: May 16, 2015, 13:49 UTC
Message-ID: <CACRoPnSP9xfyW47ZqU7QO5o4tyzROh4hGRPqG9g9OB5cquS+uw@mail.gmail.com>
In-Reply-To: <xmqq7fs9hekc.fsf@gitster.dls.corp.google.com>

Hi Junio,

On Sat, May 16, 2015 at 2:37 AM, Junio C Hamano <gitster@pobox.com> wrote:

> Just to avoid misunderstanding, please do not remove 'verbose '
> blindly without thinking while doing so, as you already did 1/3 of
> the necessary job to make things better.

Eh? I thought we established that using "verbose" does not provide anything more than what "set -x" already provides. So at the very least, its use should be removed completely.

Here is my understanding of the current situation, please correct me if I am wrong:

When a test fails, it would be useful to know the exact, unexpanded, unsubstituted command which failed (additionally with a nice stack trace and line numbers). However, the output of -v and -x (and by extension, the "verbose" function) is not very helpful, as it still requires the debugger to understand the test script.

-v will print out the stdout and stderr of the executed commands, but it requires the debugger to match up the output of the commands with the test script to understand where the test failed. It is also not very helpful in the case of "test", which does not print anything when it fails.

-x will trace the commands being executed, but the tracing output is so verbose it still requires the debugger to understand the test script. e.g:

     test "$(cat file)" "expected"

If the above test fails (e.g. the content of the file is "unexpected"), the tracing output will be:

     + cat file
     + test unexpected = expected
     error: last command exited with $?=1

Furthermore, the format of the tracing output is not specified by POSIX, so we can't count on it being consistent among all shells (e.g. for users who submit bug reports with the test output)

Show 26 quoted lines

> You might have noticed, while adding them, there were something
> common that we currently do with a bare 'test' only because we
> haven't identified common needs.  As I already said, it may be that
> we often try to see a file has a known single line content (I didn't
> check if that were the case; I am just giving you an example) and
> only because there is no ready-made test_file_contents helper to be
> used, the current tests say
>
>         test expected_string = "$(cat file)"
>
> And if that were the case, it is a good thing to have a new helper
> like this
>
>         test_file_contents () {
>                 if test "$(cat "$1")" != "$2"
>                 then
>                         echo "Contents of file '$1' is not '$2'"
>                         false
>                 fi
>         }
>
> in t/test-lib-functions.sh and convert them to say
>
>         test_file_contents file expected_string
>
> That would be an improvement (and that is the remaining 2/3 ;-).

Yeah, this kind of comparison with file contents is something that is done often in t5520, so I agree with adding it.

However, what about these kind of tests:

     test new = "$(git show HEAD:file2)"

or these:

     test $(git rev-parse HEAD^2) = $(git rev-parse keep-merge)

So, perhaps we could introduce a generic function like:

    # Compares that the output of $1 eval'ed is identical to $2.
    test_output () {
        output=$(eval $1)
        if "$output" != "$2"
        then
             echo >&2 "Output of '$1' ('$output') != '$2'"
             false
        fi
    }

So the first example would be:

    test_output "git show HEAD:file2" new

And the error output will thus be:

     Output of 'git show HEAD:file2' ('some unexpected output') != 'new'

So we know the exact comparison that failed, and we know how the expected and actual output differs.

What do you think?

Thanks, Paul