On 2017-01-05 20:31, Stefan Beller wrote:
> On Thu, Jan 5, 2017 at 5:09 PM, Richard Hansen <hansenr@google.com> 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 rhansen@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