On 2017-01-05 20:31, Stefan Beller wrote:
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. ;)
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@xxxxxxx); 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
<<attachment: smime.p7s>>