Re: [PATCH v3 4/5] commit test: test_set_editor in each test
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- May 27, 2014, 22:59 UTC
- Message-ID
- <CAPig+cTc_sSxLm0QGxY40awnr9MD5NoKgc8pH+K67wwV+2p5-Q@mail.gmail.com>
- In-Reply-To
- <1401130586-93105-5-git-send-email-caleb@calebthompson.io>
On Mon, May 26, 2014 at 2:56 PM, Caleb Thompson <cjaysson@gmail.com> wrote:
Show 5 quoted lines
> t/t7507-commit-verbose.sh was using a global test_set_editor call to > build its environment. > > Rather than building global state with test_set_editor at the beginning > of the file, move test_set_editor calls into each test.
Rather than repeating in prose what the patch itself says more concisely and precisely, explain the reason for this change. For instance, you might replace the above two sentences with something like this (or better):
Improve robustness against global state changes by having each
test set up the test-editor it requires rather than relying upon
the editor set once at script start.> Besides being inline with current practices, it also allows the tests
s/inline/in line/
> which required GIT_EDITOR=cat to avoid using a subshell and simplify > their logic.
"required" sounds odd here. Perhaps:
...which set GIT_EDITOR=cat manually...
More below.
Show 30 quoted lines
> Signed-off-by: Caleb Thompson <caleb@calebthompson.io> > --- > t/t7507-commit-verbose.sh | 22 +++++++++++----------- > 1 file changed, 11 insertions(+), 11 deletions(-) > > diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh > index e62d921..310b68b 100755 > --- a/t/t7507-commit-verbose.sh > +++ b/t/t7507-commit-verbose.sh > @@ -6,7 +6,6 @@ test_description='verbose commit template' > write_script check-for-diff <<-EOF > exec grep '^diff --git' "\$1" > EOF > -test_set_editor "$(pwd)/check-for-diff" > > cat >message <<'EOF' > subject > @@ -21,10 +20,12 @@ test_expect_success 'setup' ' > ' > > test_expect_success 'initial commit shows verbose diff' ' > + test_set_editor "$(pwd)/check-for-diff" && > git commit --amend -v > ' > > test_expect_success 'second commit' ' > + test_set_editor "$(pwd)/check-for-diff" && > echo content modified >file && > git add file && > git commit -F message
This test does not invoke the test-editor at all, so it's misleading to insert test_set_editor here.
Show 22 quoted lines
> @@ -36,11 +37,13 @@ check_message() {
> }
>
> test_expect_success 'verbose diff is stripped out' '
> + test_set_editor "$(pwd)/check-for-diff" &&
> git commit --amend -v &&
> check_message message
> '
>
> test_expect_success 'verbose diff is stripped out (mnemonicprefix)' '
> + test_set_editor "$(pwd)/check-for-diff" &&
> test_config diff.mnemonicprefix true &&
> git commit --amend -v &&
> check_message message
> @@ -59,16 +62,19 @@ index 0000000..f95c11d
> EOF
>
> test_expect_success 'diff in message is retained without -v' '
> + test_set_editor "$(pwd)/check-for-diff" &&
> git commit --amend -F diff &&
> check_message diff
> 'Also misleading, unnecessary test_set_editor.
Show 5 quoted lines
> test_expect_success 'diff in message is retained with -v' ' > + test_set_editor "$(pwd)/check-for-diff" && > git commit --amend -F diff -v && > check_message diff > '
Ditto.
> test_expect_success 'submodule log is stripped out too with -v' ' > + test_set_editor "$(pwd)/check-for-diff" &&
Unnecessary. The editor isn't invoked until the test_must_fail line, and by then you've already overridden it with 'test_set_editor cat'.
Show 14 quoted lines
> test_config diff.submodule log && > git submodule add ./. sub && > git commit -m "sub added" && > @@ -77,20 +83,14 @@ test_expect_success 'submodule log is stripped out too with -v' ' > echo "more" >>file && > git commit -a -m "submodule commit" > ) && > - ( > - GIT_EDITOR=cat && > - export GIT_EDITOR && > - test_must_fail git commit -a -v 2>err > - ) && > + test_set_editor cat && > + test_must_fail git commit -a -v 2>err
Broken &&-chain.
Show 11 quoted lines
> test_i18ngrep "Aborting commit due to empty commit message." err > ' > > test_expect_success 'verbose diff is stripped out with set core.commentChar' ' > - ( > - GIT_EDITOR=cat && > - export GIT_EDITOR && > - test_must_fail git -c core.commentchar=";" commit -a -v 2>err > - ) && > + test_set_editor cat && > + test_must_fail git -c core.commentchar=";" commit -a -v 2>err
Broken &&-chain.
Show 6 quoted lines
> test_i18ngrep "Aborting commit due to empty commit message." err > ' > > -- > 1.9.3 >