On Thu, Jan 5, 2017 at 5:09 PM, Richard Hansen <hansenr@xxxxxxxxxx> 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 rhansen@xxxxxxx); 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.