Re: [PATCH v2 2/4] t7610: make tests more independent and debuggable

[Richard Hansen]

On 2017-01-05 20:31, Stefan Beller wrote:

On Thu, Jan 5, 2017 at 5:09 PM, Richard Hansen  wrote:

Reduce how much a test can interfere other tests:


A bullet point list as an unordered list often indicates that you're
doing multiple
things at once, possibly unrelated, so they could go into different patches. ;)


OK, I'll split it up.



While telling you to make even more commits: you may also want to make
a patch with an entry to the .mailmap file (assuming you're the same
Richard Hansen that contributed from rhan...@bbn.com);
Welcome to Google!


Good idea, thanks!





  * Move setup code that multiple tests depend on to the 'setup' test
case.
  * Run 'git reset --hard' after every test (pass or fail) to clean up
in case the test fails and leaves the repository in a strange
state.
  * If the repository must be in a particular state (beyond what is
already done by the 'setup' test case) before the test can run,
make the necessary repository changes in the test script even if
it means duplicating some lines of code from the previous test
case.
  * Never assume that a particular commit is checked out.
  * Always work on a test-specific branch when the test might create a
commit.  This is not always necessary for correctness, but it
improves debuggability by ensuring a commit created by test #N
shows up on the testN branch, not the branch for test #N-1.






@@ -112,6 +146,7 @@ test_expect_success 'custom mergetool' '
 '

 test_expect_success 'mergetool crlf' '
+   test_when_finished "git reset --hard" &&
test_config core.autocrlf true &&
git checkout -b test$test_count branch1 &&
test_must_fail git merge master >/dev/null 2>&1 &&
@@ -129,11 +164,11 @@ test_expect_success 'mergetool crlf' '
git submodule update -N &&
test "$(cat submod/bar)" = "master submodule" &&
git commit -m "branch1 resolved with mergetool - autocrlf" &&



-   test_config core.autocrlf false &&
-   git reset --hard
+   test_config core.autocrlf false
 '


This is the nit that led me to writing this email in the first place:
test_config is a function that sets a configuration for a single test only,
so it makes no sense as the last statement of a test. (In its implementation
it un-configures with test_when_finished)

So I think we do not want to add it back, but rather remove this
test_config statement.


OK, will do.



But to do this we need to actually be careful with the order of the newly
added test_when_finished "git reset --hard" and  test_config core.autocrlf true,
which uses test_when_finished internally.


Ah, yes.  Tricky.  I'll add a comment.



The order seems correct to me, as the reset would be executed after the
"test_config core.autocrlf true" is un-configured.


Agreed; test_when_finished is LIFO (though the order is not documented).

-Richard


smime.p7s
Description: S/MIME Cryptographic Signature

Re: [PATCH v2 2/4] t7610: make tests more independent and debuggable

[Stefan Beller]

On Thu, Jan 5, 2017 at 5:09 PM, Richard Hansen  wrote:
> Reduce how much a test can interfere other tests:

A bullet point list as an unordered list often indicates that you're
doing multiple
things at once, possibly unrelated, so they could go into different patches. ;)

While telling you to make even more commits: you may also want to make
a patch with an entry to the .mailmap file (assuming you're the same
Richard Hansen that contributed from rhan...@bbn.com);
Welcome to Google!

>
>   * Move setup code that multiple tests depend on to the 'setup' test
> case.
>   * Run 'git reset --hard' after every test (pass or fail) to clean up
> in case the test fails and leaves the repository in a strange
> state.
>   * If the repository must be in a particular state (beyond what is
> already done by the 'setup' test case) before the test can run,
> make the necessary repository changes in the test script even if
> it means duplicating some lines of code from the previous test
> case.
>   * Never assume that a particular commit is checked out.
>   * Always work on a test-specific branch when the test might create a
> commit.  This is not always necessary for correctness, but it
> improves debuggability by ensuring a commit created by test #N
> shows up on the testN branch, not the branch for test #N-1.




> @@ -112,6 +146,7 @@ test_expect_success 'custom mergetool' '
>  '
>
>  test_expect_success 'mergetool crlf' '
> +   test_when_finished "git reset --hard" &&
> test_config core.autocrlf true &&
> git checkout -b test$test_count branch1 &&
> test_must_fail git merge master >/dev/null 2>&1 &&
> @@ -129,11 +164,11 @@ test_expect_success 'mergetool crlf' '
> git submodule update -N &&
> test "$(cat submod/bar)" = "master submodule" &&
> git commit -m "branch1 resolved with mergetool - autocrlf" &&

> -   test_config core.autocrlf false &&
> -   git reset --hard
> +   test_config core.autocrlf false
>  '

This is the nit that led me to writing this email in the first place:
test_config is a function that sets a configuration for a single test only,
so it makes no sense as the last statement of a test. (In its implementation
it un-configures with test_when_finished)

So I think we do not want to add it back, but rather remove this
test_config statement.

But to do this we need to actually be careful with the order of the newly
added test_when_finished "git reset --hard" and  test_config core.autocrlf true,
which uses test_when_finished internally.

The order seems correct to me, as the reset would be executed after the
"test_config core.autocrlf true" is un-configured.

[PATCH v2 2/4] t7610: make tests more independent and debuggable

[Richard Hansen]

Reduce how much a test can interfere other tests:

  * Move setup code that multiple tests depend on to the 'setup' test
case.
  * Run 'git reset --hard' after every test (pass or fail) to clean up
in case the test fails and leaves the repository in a strange
state.
  * If the repository must be in a particular state (beyond what is
already done by the 'setup' test case) before the test can run,
make the necessary repository changes in the test script even if
it means duplicating some lines of code from the previous test
case.
  * Never assume that a particular commit is checked out.
  * Always work on a test-specific branch when the test might create a
commit.  This is not always necessary for correctness, but it
improves debuggability by ensuring a commit created by test #N
shows up on the testN branch, not the branch for test #N-1.

Signed-off-by: Richard Hansen 
---
 t/t7610-mergetool.sh | 146 +--
 1 file changed, 84 insertions(+), 62 deletions(-)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 14090739f..2c06cf73f 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -55,6 +55,22 @@ test_expect_success 'setup' '
git rm file12 &&
git commit -m "branch1 changes" &&
 
+   git checkout -b delete-base branch1 &&
+   mkdir -p a/a &&
+   (echo one; echo two; echo 3; echo 4) >a/a/file.txt &&
+   git add a/a/file.txt &&
+   git commit -m"base file" &&
+   git checkout -b move-to-b delete-base &&
+   mkdir -p b/b &&
+   git mv a/a/file.txt b/b/file.txt &&
+   (echo one; echo two; echo 4) >b/b/file.txt &&
+   git commit -a -m"move to b" &&
+   git checkout -b move-to-c delete-base &&
+   mkdir -p c/c &&
+   git mv a/a/file.txt c/c/file.txt &&
+   (echo one; echo two; echo 3) >c/c/file.txt &&
+   git commit -a -m"move to c" &&
+
git checkout -b stash1 master &&
echo stash1 change file11 >file11 &&
git add file11 &&
@@ -86,6 +102,23 @@ test_expect_success 'setup' '
git rm file11 &&
git commit -m "master updates" &&
 
+   git clean -fdx &&
+   git checkout -b order-file-start master &&
+   echo start >a &&
+   echo start >b &&
+   git add a b &&
+   git commit -m start &&
+   git checkout -b order-file-side1 order-file-start &&
+   echo side1 >a &&
+   echo side1 >b &&
+   git add a b &&
+   git commit -m side1 &&
+   git checkout -b order-file-side2 order-file-start &&
+   echo side2 >a &&
+   echo side2 >b &&
+   git add a b &&
+   git commit -m side2 &&
+
git config merge.tool mytool &&
git config mergetool.mytool.cmd "cat \"\$REMOTE\" >\"\$MERGED\"" &&
git config mergetool.mytool.trustExitCode true &&
@@ -94,6 +127,7 @@ test_expect_success 'setup' '
 '
 
 test_expect_success 'custom mergetool' '
+   test_when_finished "git reset --hard" &&
git checkout -b test$test_count branch1 &&
git submodule update -N &&
test_must_fail git merge master >/dev/null 2>&1 &&
@@ -112,6 +146,7 @@ test_expect_success 'custom mergetool' '
 '
 
 test_expect_success 'mergetool crlf' '
+   test_when_finished "git reset --hard" &&
test_config core.autocrlf true &&
git checkout -b test$test_count branch1 &&
test_must_fail git merge master >/dev/null 2>&1 &&
@@ -129,11 +164,11 @@ test_expect_success 'mergetool crlf' '
git submodule update -N &&
test "$(cat submod/bar)" = "master submodule" &&
git commit -m "branch1 resolved with mergetool - autocrlf" &&
-   test_config core.autocrlf false &&
-   git reset --hard
+   test_config core.autocrlf false
 '
 
 test_expect_success 'mergetool in subdir' '
+   test_when_finished "git reset --hard" &&
git checkout -b test$test_count branch1 &&
git submodule update -N &&
(
@@ -145,8 +180,13 @@ test_expect_success 'mergetool in subdir' '
 '
 
 test_expect_success 'mergetool on file in parent dir' '
+   test_when_finished "git reset --hard" &&
+   git checkout -b test$test_count branch1 &&
+   git submodule update -N &&
(
cd subdir &&
+   test_must_fail git merge master >/dev/null 2>&1 &&
+   ( yes "" | git mergetool file3 >/dev/null 2>&1 ) &&
( yes "" | git mergetool ../file1 >/dev/null 2>&1 ) &&
( yes "" | git mergetool ../file2 ../spaced\ name >/dev/null 
2>&1 ) &&
( yes "" | git mergetool ../both >/dev/null 2>&1 ) &&
@@ -161,6 +201,7 @@ test_expect_success 'mergetool on file in parent dir' '
 '
 
 test_expect_success 'mergetool skips autoresolved' '
+   test_when_finished "git reset --hard" &&
git checkout -b test$test_count branch1 &&
git submodule update -N &&
test_must_fail git merge master &&
@@ -169,11