Re: [PATCH 2/4] t7610: make tests more independent and debuggable
On 2017-01-04 15:27, Stefan Beller wrote: On Tue, Jan 3, 2017 at 4:50 PM, Richard Hansenwrote: If a test fails it might leave the repository in a strange state. Add 'git reset --hard' at the beginning of each test to increase the odds of passing when an earlier test fails. So each test is cleaning up the previous test, which *may* confuse a reader ("how is the reset --hard relevant for this test? Oooh it's just a cleanup"). We could put it another way by having each test itself make clean up after itself via test_when_finished "git reset --hard" && .. at the beginning of each test. This would produce the same order of operations, i.e. a reset run between each test, but semantically tells the reader that the reset is part of the current test cleaning up after itself, as "reset" is operation for this particular test to cleanup. Does that make sense? I like that idea; thanks for the suggestion. I'll cook up a reroll. Also use test-specific branches to avoid interfering with later tests and to make the tests easier to debug. That sounds great! Though in the code I only spot one occurrence for + git checkout -b test$test_count branch1 && so maybe that could be part of the first patch in the series? There are two; the other is buried in the change for the 'mergetool on file in parent dir' test. Thanks for the review, Richard Thanks, Stefan
Re: [PATCH 2/4] t7610: make tests more independent and debuggable
On 2017-01-05 07:20, Simon Ruderich wrote: On Tue, Jan 03, 2017 at 07:50:40PM -0500, Richard Hansen wrote: [snip] @@ -145,8 +148,13 @@ test_expect_success 'mergetool in subdir' ' ' test_expect_success 'mergetool on file in parent dir' ' + 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 ) && This change seems unrelated to the changes mentioned in the commit message. Was it intended? Yes, that is intentional; without this change, the test depends on the successful completion of the previous test. I'll improve the commit message. Thanks, Richard Regards Simon smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH 2/4] t7610: make tests more independent and debuggable
On 2017-01-04 15:27, Stefan Beller wrote: On Tue, Jan 3, 2017 at 4:50 PM, Richard Hansenwrote: If a test fails it might leave the repository in a strange state. Add 'git reset --hard' at the beginning of each test to increase the odds of passing when an earlier test fails. So each test is cleaning up the previous test, which *may* confuse a reader ("how is the reset --hard relevant for this test? Oooh it's just a cleanup"). We could put it another way by having each test itself make clean up after itself via test_when_finished "git reset --hard" && .. at the beginning of each test. This would produce the same order of operations, i.e. a reset run between each test, but semantically tells the reader that the reset is part of the current test cleaning up after itself, as "reset" is operation for this particular test to cleanup. Does that make sense? I like that idea; thanks for the suggestion. I'll cook up a reroll. Also use test-specific branches to avoid interfering with later tests and to make the tests easier to debug. That sounds great! Though in the code I only spot one occurrence for + git checkout -b test$test_count branch1 && so maybe that could be part of the first patch in the series? There are two; the other is buried in the change for the 'mergetool on file in parent dir' test. I'll improve the commit message to make it more clear. Thanks for the review, Richard Thanks, Stefan smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH 2/4] t7610: make tests more independent and debuggable
On Tue, Jan 03, 2017 at 07:50:40PM -0500, Richard Hansen wrote: > [snip] > @@ -145,8 +148,13 @@ test_expect_success 'mergetool in subdir' ' > ' > > test_expect_success 'mergetool on file in parent dir' ' > + 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 ) && This change seems unrelated to the changes mentioned in the commit message. Was it intended? Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 signature.asc Description: PGP signature
Re: [PATCH 2/4] t7610: make tests more independent and debuggable
On Tue, Jan 3, 2017 at 4:50 PM, Richard Hansenwrote: > If a test fails it might leave the repository in a strange state. Add > 'git reset --hard' at the beginning of each test to increase the odds > of passing when an earlier test fails. So each test is cleaning up the previous test, which *may* confuse a reader ("how is the reset --hard relevant for this test? Oooh it's just a cleanup"). We could put it another way by having each test itself make clean up after itself via test_when_finished "git reset --hard" && .. at the beginning of each test. This would produce the same order of operations, i.e. a reset run between each test, but semantically tells the reader that the reset is part of the current test cleaning up after itself, as "reset" is operation for this particular test to cleanup. Does that make sense? > > Also use test-specific branches to avoid interfering with later tests > and to make the tests easier to debug. That sounds great! Though in the code I only spot one occurrence for + git checkout -b test$test_count branch1 && so maybe that could be part of the first patch in the series? Thanks, Stefan