| Message ID | 20240229150442.490649-2-shejialuo@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | microproject: Use test_path_is_* functions in test scripts | expand |
On Thu, Feb 29, 2024 at 10:05 AM shejialuo <shejialuo@gmail.com> wrote: > t3070: refactor test -e command > > The "test_path_exists" function was proposed at 7e9055b. It provides > parameter number check and more robust error messages. > > This patch converts all "test -e" into "test_path_exists" to improve > test debug when failure. Thanks for providing this GSoC submission. The aim of this patch makes sense, but it turns out that t3070 is not a good choice for this exercise. Before getting into that, though, a few minor comments about the commit message. This patch isn't actually refactoring the code, so using "refactor" in the title is misleading. Rather than mentioning only the object-ID, we normally reference other commits like this (using `git log --pretty=reference -1 <object-id>`): 7e9055bb00 (t7406: prefer test_* helper functions to test -[feds], 2018-08-08) In this case, it's not clear why you chose to reference that particular commit over any of the others which make similar changes. It probably would be simpler to drop mention of that commit and just copy its reasoning into your commit message. Taking all the above into account, a possible rewrite of the commit message might be: t3070: prefer test_path_exists helper function test -e does not provide a nice error message when we hit test failures, so use test_path_exists instead. > Signed-off-by: shejialuo <shejialuo@gmail.com> > --- > diff --git a/t/t3070-wildmatch.sh b/t/t3070-wildmatch.sh > @@ -107,7 +107,7 @@ match_with_ls_files() { > if test "$match_expect" = 'E' > then > - if test -e .git/created_test_file > + if test_path_exists .git/created_test_file > then > test_expect_success EXPENSIVE_ON_WINDOWS "$match_function (via ls-files): match dies on '$pattern' '$text'" " The point of functions such as test_path_exists() is to _assert_ that some condition is true, thus allowing the test to succeed; if the condition is not true, then the function prints an error message and the test aborts and fails. Here is how test_path_exists() is defined: test_path_exists () { test "$#" -ne 1 && BUG "1 param" if ! test -e "$1" then echo "Path $1 doesn't exist" false fi } It is meant to replace noisy code such as: if ! test -e bloop then echo >&2 "error message" && exit 1 fi && other-code with much simpler: test_path_exists bloop && other-code It is also meant to be used within `test_expect_success` (or `test_expect_failure`) blocks. So, the changes made by this patch are undesirable for a couple reasons... First, this code is outside a `test_expect_success` (or `test_expect_failure`) block. Second, as noted above, test_path_exists() is an _assertion_ which requires the file to exist, and aborts the test if the file does not exist. But the `test -e` being changed here is part of the proper control-flow of this logic; it is not asserting anything, but merely branching to one or another part of the code depending upon the result of the `test -e` test. Thus, replacing this control-flow check with the assertion function test_path_exists() changes the logic in an undesirable way. The above comments are applicable to most of the changes made by this patch. The only exceptions are the last two changes... > @@ -175,7 +175,7 @@ match() { > test_expect_success EXPENSIVE_ON_WINDOWS 'cleanup after previous file test' ' > - if test -e .git/created_test_file > + if test_path_exists .git/created_test_file > then > git reset && ... which _do_ use test_path_exists() within a `test_expect_success` block. However, the changes are still undesirable because, as above, this `test -e` is merely part of the normal control-flow; it's not acting as an assertion, thus test_path_exists() -- which is an assertion -- is not correct. Unfortunately, none of the uses of`test -e` in t3070 are being used as assertions worthy of replacement with test_path_exists(), thus this isn't a good script in which to make such changes. If you reroll, you may be able to find a good candidate script by searching for code which looks something like this: foo && test -e path && bar && and replacing it with: foo && test_path_exists path && bar &&
Eric Sunshine <sunshine@sunshineco.com> writes: >> @@ -175,7 +175,7 @@ match() { >> test_expect_success EXPENSIVE_ON_WINDOWS 'cleanup after previous file test' ' >> - if test -e .git/created_test_file >> + if test_path_exists .git/created_test_file >> then >> git reset && > > ... which _do_ use test_path_exists() within a `test_expect_success` > block. However, the changes are still undesirable because, as above, > this `test -e` is merely part of the normal control-flow; it's not > acting as an assertion, thus test_path_exists() -- which is an > assertion -- is not correct. > > Unfortunately, none of the uses of`test -e` in t3070 are being used as > assertions worthy of replacement with test_path_exists(), thus this > isn't a good script in which to make such changes. It seems that there is a recurring confusion among mentorship program applicants that use test_path_* helpers as their practice material. Perhaps the source of the information that suggests it as a microproject is poorly phrased and needs to be rewritten to avoid misleading them. I found one at https://git.github.io/Outreachy-23-Microprojects/, which can be one source of such confusion: Find one test script that verifies the presence/absence of files/directories with ‘test -(e|f|d|…)’ and replace them with the appropriate test_path_is_file, test_path_is_dir, etc. helper functions. but there may be others. This task specification does not differenciate "test -[efdx]" used as a conditional of a control flow statement (which should never be replaced by test_path_* helpers) and those used to directly fail the &&-chain in test_expect_success with their exit status (which is the target that test_path_* helpers are meant to improve).
Thanks for your comment, I will find a candidate script later and submit a new version patch.
On Thu, Feb 29, 2024 at 11:06:41AM -0800, Junio C Hamano wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > > >> @@ -175,7 +175,7 @@ match() { > >> test_expect_success EXPENSIVE_ON_WINDOWS 'cleanup after previous file test' ' > >> - if test -e .git/created_test_file > >> + if test_path_exists .git/created_test_file > >> then > >> git reset && > > > > ... which _do_ use test_path_exists() within a `test_expect_success` > > block. However, the changes are still undesirable because, as above, > > this `test -e` is merely part of the normal control-flow; it's not > > acting as an assertion, thus test_path_exists() -- which is an > > assertion -- is not correct. > > > > Unfortunately, none of the uses of`test -e` in t3070 are being used as > > assertions worthy of replacement with test_path_exists(), thus this > > isn't a good script in which to make such changes. > > It seems that there is a recurring confusion among mentorship > program applicants that use test_path_* helpers as their practice > material. Perhaps the source of the information that suggests it as > a microproject is poorly phrased and needs to be rewritten to avoid > misleading them. > > I found one at https://git.github.io/Outreachy-23-Microprojects/, > which can be one source of such confusion: > > Find one test script that verifies the presence/absence of > files/directories with ‘test -(e|f|d|…)’ and replace them > with the appropriate test_path_is_file, test_path_is_dir, > etc. helper functions. > > but there may be others. > > This task specification does not differenciate "test -[efdx]" used > as a conditional of a control flow statement (which should never be > replaced by test_path_* helpers) and those used to directly fail the > &&-chain in test_expect_success with their exit status (which is the > target that test_path_* helpers are meant to improve). Good point. I've sent a patch in reply to your message that hopefully clarifies this a bit. Thanks! Patrick
diff --git a/t/t3070-wildmatch.sh b/t/t3070-wildmatch.sh index 4dd42df38c..d18ddc1a52 100755 --- a/t/t3070-wildmatch.sh +++ b/t/t3070-wildmatch.sh @@ -107,7 +107,7 @@ match_with_ls_files() { if test "$match_expect" = 'E' then - if test -e .git/created_test_file + if test_path_exists .git/created_test_file then test_expect_success EXPENSIVE_ON_WINDOWS "$match_function (via ls-files): match dies on '$pattern' '$text'" " printf '%s' '$text' >expect && @@ -118,7 +118,7 @@ match_with_ls_files() { fi elif test "$match_expect" = 1 then - if test -e .git/created_test_file + if test_path_exists .git/created_test_file then test_expect_success EXPENSIVE_ON_WINDOWS "$match_function (via ls-files): match '$pattern' '$text'" " printf '%s' '$text' >expect && @@ -130,7 +130,7 @@ match_with_ls_files() { fi elif test "$match_expect" = 0 then - if test -e .git/created_test_file + if test_path_exists .git/created_test_file then test_expect_success EXPENSIVE_ON_WINDOWS "$match_function (via ls-files): no match '$pattern' '$text'" " >expect && @@ -175,7 +175,7 @@ match() { fi test_expect_success EXPENSIVE_ON_WINDOWS 'cleanup after previous file test' ' - if test -e .git/created_test_file + if test_path_exists .git/created_test_file then git reset && git clean -df @@ -198,7 +198,7 @@ match() { fi && git add -A && printf "%s" "$file" >.git/created_test_file - elif test -e .git/created_test_file + elif test_path_exists .git/created_test_file then rm .git/created_test_file fi
The "test_path_exists" function was proposed at 7e9055b. It provides parameter number check and more robust error messages. This patch converts all "test -e" into "test_path_exists" to improve test debug when failure. Signed-off-by: shejialuo <shejialuo@gmail.com> --- t/t3070-wildmatch.sh | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)