Re: [PATCH v2 2/4] t7610: make tests more independent and debuggable
On 2017-01-05 20:31, Stefan Beller wrote:
Show 6 quoted lines
> 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. ;)
Show 5 quoted lines
>
> 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!
Show 46 quoted lines
>
>>
>> * 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.
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