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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Mar 24, 2016 at 7:00 AM, SZEDER Gábor <szeder@xxxxxxxxxx> wrote:
>> 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.

>> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]