Re: [PATCH v9 2/3] t7507-commit-verbose: make test suite use write_script

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 24, 2016, 23:57 UTC
Message-ID: <CAPig+cQJ1yK8WhXE0sxSmDenMt1DMZCoJypTu_wkDsDyuBsAWg@mail.gmail.com>
In-Reply-To: <1458817259-11675-1-git-send-email-szeder@ira.uka.de>

On Thu, Mar 24, 2016 at 7:00 AM, SZEDER Gábor <szeder@ira.uka.de> wrote:

Show 8 quoted lines

>> Also remove test_set_editor from global scope and use it in whichever
>> test it is required.
>
> Why?
>
> test_set_editor sets and exports shell variables.  Since you don't
> invoke test_set_editor in a subshell, after the first invocation the
> editor will be part of the global scope anyway.

Agreed that this needs proper justification in the commit message. And, the justification is to make each test more self-contained, particularly because a subsequent patch will introduce a second fake "editor", and by making tests responsible for setting the editor they need, they don't have to worry about bad interactions from "editors" set by earlier tests[1][2].

Another issue is that the commit message is backward. The subject ("t7507-commit-verbose: make test suite use write_script") tries to sell this as primarily being about write_script(), but the real gist of the patch is that it is making each test more self-contained, and the subject should say so. The write_script() bit is just a minor aside which can be introduced with a "While here let's use write_script to..." at the end of the commit message.

Show 18 quoted lines

>> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
>> index 2ddf28c..cf95efb 100755
>> --- a/t/t7507-commit-verbose.sh
>> +++ b/t/t7507-commit-verbose.sh
>> @@ -3,12 +3,11 @@
>>  test_description='verbose commit template'
>>  . ./test-lib.sh
>>
>> -cat >check-for-diff <<EOF
>> -#!$SHELL_PATH
>> -exec grep '^diff --git' "\$1"
>> +write_script "check-for-diff" <<-\EOF &&
>> +grep '^diff --git' "$1" >out &&
>> +test $(wc -l <out) = 1
>
> Our test lib offers the test_line_count helper function, which
> outputs a helpful error message in case the number of lines do not
> match.

test_line_count() was mentioned in [2], however, this line counting is done in the fake "editor" script, not in the test script proper, so the spelled-out form $(wc ...) was proposed[2].

> The original didn't check the number of lines.  This change is not
> mentioned at all in the commit message.

Right, and this particular change really belongs in patch 3/3 where it's needed[2], and should be properly explained by the 3/3 commit message.

>>  EOF
>>  chmod +x check-for-diff

Drop the 'chmod' line; write_script() does this for you.

>> -test_set_editor "$PWD/check-for-diff"
>>
>>  cat >message <<'EOF'
>>  subject

[1]: http://article.gmane.org/gmane.comp.version-control.git/289329 [2]: http://article.gmane.org/gmane.comp.version-control.git/289370