| Message ID | 83e47748bc9c541c725f6c42c553b1a69fd717ac.1576794144.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t: replace incorrect test_must_fail usage (part 1) | expand |
On Thu, Dec 19, 2019 at 5:22 PM Denton Liu <liu.denton@gmail.com> wrote: > The test_must_fail() family of functions (including test_might_fail()) > should only be used on git commands. Replace test_might_fail() with > test_non_git_might_fail(). > > The `test_might_fail cp` line was introduced in 466e8d5d66 (t1501: fix > test with split index, 2015-03-24). It is necessary because there might > exist some index files in `repo.git/sharedindex.*` and, if they exist, > we want to copy them over. However, if they don't exist, we don't want > to error out because we expect that possibility. As a result, we want to > keep the "might fail" semantics so we use test_non_git_might_fail(). Thanks for adding this paragraph to help the reader understand why you chose this transformation. However... > Signed-off-by: Denton Liu <liu.denton@gmail.com> > --- > diff --git a/t/t1501-work-tree.sh b/t/t1501-work-tree.sh > @@ -350,7 +350,7 @@ test_expect_success 'Multi-worktree setup' ' > cp repo.git/HEAD repo.git/index repo.git/repos/foo && > - test_might_fail cp repo.git/sharedindex.* repo.git/repos/foo && > + test_non_git_might_fail cp repo.git/sharedindex.* repo.git/repos/foo && I can't say I'm a fan of patch 1 introducing the function test_non_git_might_fail() for this one particular case. I'd rather see this case follow existing precedence by being written this way: { cp repo.git/sharedindex.* repo.git/repos/foo || :; } && which is the idiomatic way this sort of thing is handled in existing tests. While it's true that it may look a bit cryptic to people new to shell scripting, as with any idiom, it's understood at a glance by people familiar with it. That bit about "at a glance" is important: it's much easier to comprehend idiomatic code than code which you have to spend a lot of time _reading_ (and "test_non_git_might_fail" is quite a mouthful, or eyeful, or something, which takes a lot more effort to read and understand).
Hi Eric, On Thu, Dec 19, 2019 at 05:52:56PM -0500, Eric Sunshine wrote: > > diff --git a/t/t1501-work-tree.sh b/t/t1501-work-tree.sh > > @@ -350,7 +350,7 @@ test_expect_success 'Multi-worktree setup' ' > > cp repo.git/HEAD repo.git/index repo.git/repos/foo && > > - test_might_fail cp repo.git/sharedindex.* repo.git/repos/foo && > > + test_non_git_might_fail cp repo.git/sharedindex.* repo.git/repos/foo && > > I can't say I'm a fan of patch 1 introducing the function > test_non_git_might_fail() for this one particular case. I'd rather see > this case follow existing precedence by being written this way: > > { cp repo.git/sharedindex.* repo.git/repos/foo || :; } && > > which is the idiomatic way this sort of thing is handled in existing tests. > > While it's true that it may look a bit cryptic to people new to shell > scripting, as with any idiom, it's understood at a glance by people > familiar with it. That bit about "at a glance" is important: it's much > easier to comprehend idiomatic code than code which you have to spend > a lot of time _reading_ (and "test_non_git_might_fail" is quite a > mouthful, or eyeful, or something, which takes a lot more effort to > read and understand). The reason why I chose to do this was because I found myself writing the above many times in (currently unsent) later test cases that I cleaned up. As a result, I felt like it could be wrapped up more nicely with a helper function. That being said, if you think that open coding the idiom looks nicer, I can reroll to inline it.
On Thu, Dec 19, 2019 at 6:18 PM Denton Liu <liu.denton@gmail.com> wrote: > On Thu, Dec 19, 2019 at 05:52:56PM -0500, Eric Sunshine wrote: > > [...] I'd rather see: > > { cp repo.git/sharedindex.* repo.git/repos/foo || :; } && > > which is the idiomatic way this sort of thing is handled in existing tests. > > > > While it's true that it may look a bit cryptic to people new to shell > > scripting, as with any idiom, it's understood at a glance by people > > familiar with it. That bit about "at a glance" is important: it's much > > easier to comprehend idiomatic code than code which you have to spend > > a lot of time _reading_ [...] > > The reason why I chose to do this was because I found myself writing the > above many times in (currently unsent) later test cases that I cleaned > up. As a result, I felt like it could be wrapped up more nicely with a > helper function. That being said, if you think that open coding the > idiom looks nicer, I can reroll to inline it. I wouldn't say that the idiom "looks nicer", but rather that it has value specifically because it is an idiom, therefore it reduces cognitive load (as explained above). Idioms aside, the new function test_non_git_might_fail() may increase cognitive load because the reader needs to remember its purpose and behavior since it's a black box compared to the open-coded version, yet adds no actual value. Compare that with, say, test_must_fail() which not only adds value but would significantly increase cognitive load if open-coded, thus is well justified. That's not to say that one-liner functions can't necessarily have value; a well named function or one which introduces an important abstraction may very well be worthwhile, but I can't convince myself that test_non_git_might_fail() succeeds in that way. So, to answer your question, my preference leans toward open-coding this.
diff --git a/t/t1501-work-tree.sh b/t/t1501-work-tree.sh index 3498d3d55e..067301a5ab 100755 --- a/t/t1501-work-tree.sh +++ b/t/t1501-work-tree.sh @@ -350,7 +350,7 @@ test_expect_success 'Multi-worktree setup' ' mkdir work && mkdir -p repo.git/repos/foo && cp repo.git/HEAD repo.git/index repo.git/repos/foo && - test_might_fail cp repo.git/sharedindex.* repo.git/repos/foo && + test_non_git_might_fail cp repo.git/sharedindex.* repo.git/repos/foo && sane_unset GIT_DIR GIT_CONFIG GIT_WORK_TREE '
The test_must_fail() family of functions (including test_might_fail()) should only be used on git commands. Replace test_might_fail() with test_non_git_might_fail(). The `test_might_fail cp` line was introduced in 466e8d5d66 (t1501: fix test with split index, 2015-03-24). It is necessary because there might exist some index files in `repo.git/sharedindex.*` and, if they exist, we want to copy them over. However, if they don't exist, we don't want to error out because we expect that possibility. As a result, we want to keep the "might fail" semantics so we use test_non_git_might_fail(). Signed-off-by: Denton Liu <liu.denton@gmail.com> --- t/t1501-work-tree.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)