t7800: fix quoting of looped test bodies

PATCH Superseded Patrick Steinhardt 2 participants 13 messages Mar 21, 2024 – Mar 22, 2024

lore

Share & tools

lore

Patrick Steinhardt

author

2024-03-21 13:47 UTC #0 lore raw

cover t7800: fix quoting of looped test bodies

Hi,

SZEDER made me aware [1] of a quoting issue with some test refactorings in t7800 I recently introduced when refactoring the tests to run in a loop. This patch series fixes those issues and also adds a paragraph to "t/README" to document how this is supposed to be done properly.

Thanks!

Patrick

[1]: https://lore.kernel.org/git/[email protected]/

Patrick Steinhardt (3):
t7800: improve test descriptions with empty arguments
t7800: use single quotes for test bodies t/README: document how to loop around test cases

 t/README            | 19 +++++++++++++++++++
 t/t7800-difftool.sh | 40 ++++++++++++++++++++--------------------
 2 files changed, 39 insertions(+), 20 deletions(-)

Signature · 1 line

2.44.0

Patrick Steinhardt

author

2024-03-21 13:47 UTC #1 lore raw

PATCH 1/3 t7800: improve test descriptions with empty arguments

Some of the tests in t7800 are executed repeatedly in a loop with different arguments. To distinguish these tests, the value of that variable is rendered into the test title. But given that one of the values is the empty string, it results in a somewhat awkward test name:

difftool ignores exit code

Improve this by printing "without options" in case the value is empty.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 t/t7800-difftool.sh | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

t/t7800-difftool.sh +8 -8

index 96ae5d5880..80bf108f54 100755
@@ -93,40 +93,40 @@ test_expect_success 'difftool forwards arguments to diff' '
 
 for opt in '' '--dir-diff'
 do
-	test_expect_success "difftool ${opt} ignores exit code" "
+	test_expect_success "difftool ${opt:-without options} ignores exit code" "
 		test_config difftool.error.cmd false &&
 		git difftool ${opt} -y -t error branch
 	"
 
-	test_expect_success "difftool ${opt} forwards exit code with --trust-exit-code" "
+	test_expect_success "difftool ${opt:-without options} forwards exit code with --trust-exit-code" "
 		test_config difftool.error.cmd false &&
 		test_must_fail git difftool ${opt} -y --trust-exit-code -t error branch
 	"
 
-	test_expect_success "difftool ${opt} forwards exit code with --trust-exit-code for built-ins" "
+	test_expect_success "difftool ${opt:-without options} forwards exit code with --trust-exit-code for built-ins" "
 		test_config difftool.vimdiff.path false &&
 		test_must_fail git difftool ${opt} -y --trust-exit-code -t vimdiff branch
 	"
 
-	test_expect_success "difftool ${opt} honors difftool.trustExitCode = true" "
+	test_expect_success "difftool ${opt:-without options} honors difftool.trustExitCode = true" "
 		test_config difftool.error.cmd false &&
 		test_config difftool.trustExitCode true &&
 		test_must_fail git difftool ${opt} -y -t error branch
 	"
 
-	test_expect_success "difftool ${opt} honors difftool.trustExitCode = false" "
+	test_expect_success "difftool ${opt:-without options} honors difftool.trustExitCode = false" "
 		test_config difftool.error.cmd false &&
 		test_config difftool.trustExitCode false &&
 		git difftool ${opt} -y -t error branch
 	"
 
-	test_expect_success "difftool ${opt} ignores exit code with --no-trust-exit-code" "
+	test_expect_success "difftool ${opt:-without options} ignores exit code with --no-trust-exit-code" "
 		test_config difftool.error.cmd false &&
 		test_config difftool.trustExitCode true &&
 		git difftool ${opt} -y --no-trust-exit-code -t error branch
 	"
 
-	test_expect_success "difftool ${opt} stops on error with --trust-exit-code" "
+	test_expect_success "difftool ${opt:-without options} stops on error with --trust-exit-code" "
 		test_when_finished 'rm -f for-diff .git/fail-right-file' &&
 		test_when_finished 'git reset -- for-diff' &&
 		write_script .git/fail-right-file <<-\EOF &&

@@ -140,7 +140,7 @@ do
 		test_line_count = 1 actual
 	"
 
-	test_expect_success "difftool ${opt} honors exit status if command not found" "
+	test_expect_success "difftool ${opt:-without options} honors exit status if command not found" "
 		test_config difftool.nonexistent.cmd i-dont-exist &&
 		test_config difftool.trustExitCode false &&
 		if test "${opt}" = '--dir-diff'

Signature · 1 line

2.44.0

Patrick Steinhardt

author

2024-03-21 13:47 UTC #2 lore raw

PATCH 2/3 t7800: use single quotes for test bodies

In reply to Patrick Steinhardt

In eb84c8b6ce (git-difftool--helper: honor --trust-exit-code with --dir-diff, 2024-02-20) we have started to loop around some of the tests in t7800 so that they are reexecuted with slightly different arguments. As part of that refactoring the quoting of test bodies was changed from single quotes (') to double quotes (") so that the value of the loop variable is accessible to the body.

As the test body is later on passed to eval this change was not required though. Let's revert it back to use single quotes as usual in our tests.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 t/t7800-difftool.sh | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

t/t7800-difftool.sh +20 ~~-20~~

index 80bf108f54..cc917b257e 100755
@@ -93,42 +93,42 @@ test_expect_success 'difftool forwards arguments to diff' '
 
 for opt in '' '--dir-diff'
 do
-	test_expect_success "difftool ${opt:-without options} ignores exit code" "
+	test_expect_success "difftool ${opt:-without options} ignores exit code" '
 		test_config difftool.error.cmd false &&
 		git difftool ${opt} -y -t error branch
-	"
+	'
 
-	test_expect_success "difftool ${opt:-without options} forwards exit code with --trust-exit-code" "
+	test_expect_success "difftool ${opt:-without options} forwards exit code with --trust-exit-code" '
 		test_config difftool.error.cmd false &&
 		test_must_fail git difftool ${opt} -y --trust-exit-code -t error branch
-	"
+	'
 
-	test_expect_success "difftool ${opt:-without options} forwards exit code with --trust-exit-code for built-ins" "
+	test_expect_success "difftool ${opt:-without options} forwards exit code with --trust-exit-code for built-ins" '
 		test_config difftool.vimdiff.path false &&
 		test_must_fail git difftool ${opt} -y --trust-exit-code -t vimdiff branch
-	"
+	'
 
-	test_expect_success "difftool ${opt:-without options} honors difftool.trustExitCode = true" "
+	test_expect_success "difftool ${opt:-without options} honors difftool.trustExitCode = true" '
 		test_config difftool.error.cmd false &&
 		test_config difftool.trustExitCode true &&
 		test_must_fail git difftool ${opt} -y -t error branch
-	"
+	'
 
-	test_expect_success "difftool ${opt:-without options} honors difftool.trustExitCode = false" "
+	test_expect_success "difftool ${opt:-without options} honors difftool.trustExitCode = false" '
 		test_config difftool.error.cmd false &&
 		test_config difftool.trustExitCode false &&
 		git difftool ${opt} -y -t error branch
-	"
+	'
 
-	test_expect_success "difftool ${opt:-without options} ignores exit code with --no-trust-exit-code" "
+	test_expect_success "difftool ${opt:-without options} ignores exit code with --no-trust-exit-code" '
 		test_config difftool.error.cmd false &&
 		test_config difftool.trustExitCode true &&
 		git difftool ${opt} -y --no-trust-exit-code -t error branch
-	"
+	'
 
-	test_expect_success "difftool ${opt:-without options} stops on error with --trust-exit-code" "
-		test_when_finished 'rm -f for-diff .git/fail-right-file' &&
-		test_when_finished 'git reset -- for-diff' &&
+	test_expect_success "difftool ${opt:-without options} stops on error with --trust-exit-code" '
+		test_when_finished "rm -f for-diff .git/fail-right-file" &&
+		test_when_finished "git reset -- for-diff" &&
 		write_script .git/fail-right-file <<-\EOF &&
 		echo failed
 		exit 1

@@ -138,19 +138,19 @@ do
 		test_must_fail git difftool ${opt} -y --trust-exit-code \
 			--extcmd .git/fail-right-file branch >actual &&
 		test_line_count = 1 actual
-	"
+	'
 
-	test_expect_success "difftool ${opt:-without options} honors exit status if command not found" "
+	test_expect_success "difftool ${opt:-without options} honors exit status if command not found" '
 		test_config difftool.nonexistent.cmd i-dont-exist &&
 		test_config difftool.trustExitCode false &&
-		if test "${opt}" = '--dir-diff'
+		if test "${opt}" = --dir-diff
 		then
 			expected_code=127
 		else
 			expected_code=128
 		fi &&
-		test_expect_code \${expected_code} git difftool ${opt} -y -t nonexistent branch
-	"
+		test_expect_code ${expected_code} git difftool ${opt} -y -t nonexistent branch
+	'
 done
 
 test_expect_success 'difftool honors --gui' '

Signature · 1 line

2.44.0

Patrick Steinhardt

author

2024-03-21 13:47 UTC #3 lore raw

PATCH 3/3 t/README: document how to loop around test cases

In reply to Patrick Steinhardt

In some cases it makes sense to loop around test cases so that we can execute the same test with slightly different arguments. There are some gotchas around quoting here though that are easy to miss and that may lead to easy-to-miss errors and portability issues.

Document the proper way to do this in "t/README".

Signed-off-by Patrick Steinhardt <[email protected]>

---
 t/README | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

t/README +19 -0

index 36463d0742..d56401a254 100644
@@ -721,6 +721,25 @@ The "do's:"
    Note that we still &&-chain the loop to propagate failures from
    earlier commands.
 
+ - Repeat tests with slightly different arguments in a loop.
+
+   In some cases it may make sense to re-run the same set of tests with
+   different options or commands to ensure that the command behaves
+   despite the different parameters. This can be achieved by looping
+   around a specific parameter:
+
+	for arg in '' "--foo"
+	do
+		test_expect_success "test command ${arg:-without arguments}" '
+			command $arg
+		'
+	done
+
+  Note that while the test title uses double quotes ("), the test body
+  should continue to use single quotes ('). The loop variable will be
+  accessible regardless of the single quotes as the test body is passed
+  to `eval`.
+
 
 And here are the "don'ts:"

Signature · 1 line

2.44.0

Junio C Hamano

2024-03-21 17:12 UTC #4 lore raw

Re: [PATCH 3/3] t/README: document how to loop around test cases

Quoted 28 lines · Patrick Steinhardt

In some cases it makes sense to loop around test cases so that we can execute the same test with slightly different arguments. There are some gotchas around quoting here though that are easy to miss and that may lead to easy-to-miss errors and portability issues.
Document the proper way to do this in "t/README".
Signed-off-by Patrick Steinhardt <[email protected]>
---
 t/README | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
t/README +8 -0
index 36463d0742..d56401a254 100644
@@ -721,6 +721,25 @@ The "do's:"
    Note that we still &&-chain the loop to propagate failures from
    earlier commands.
 
+ - Repeat tests with slightly different arguments in a loop.
+
+   In some cases it may make sense to re-run the same set of tests with
+   different options or commands to ensure that the command behaves
+   despite the different parameters. This can be achieved by looping
+   around a specific parameter:
+
+	for arg in '' "--foo"

t/README The "do's:" · line 741

Quoted 10 lines

t/README +10 -0

+	do
+		test_expect_success "test command ${arg:-without arguments}" '
+			command $arg
+		'
+	done
+
+  Note that while the test title uses double quotes ("), the test body
+  should continue to use single quotes ('). The loop variable will be
+  accessible regardless of the single quotes as the test body is passed
+  to `eval`.

We also want to say that they are not equivalent, don't we?

for var in '' a 'b"c'
do
test_expect_success "with dq <$var>" "
echo \"$var\"
"
done

breaks, but if we use

                test_expect_success "with sq <$var>" '
                        echo "$var"
                '

in the loop, it works as expected.

Other than that, all three patches do make sense.

Thanks.

Patrick Steinhardt

author

2024-03-22 02:05 UTC #5 lore raw

Re: [PATCH 3/3] t/README: document how to loop around test cases

Quoted 50 lines · Junio C Hamano

Quoted 28 lines · Patrick Steinhardt
In some cases it makes sense to loop around test cases so that we can execute the same test with slightly different arguments. There are some gotchas around quoting here though that are easy to miss and that may lead to easy-to-miss errors and portability issues.
Document the proper way to do this in "t/README".
Signed-off-by Patrick Steinhardt <[email protected]>
---
 t/README | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
t/README +8 -0
index 36463d0742..d56401a254 100644
@@ -721,6 +721,25 @@ The "do's:"
    Note that we still &&-chain the loop to propagate failures from
    earlier commands.
 
+ - Repeat tests with slightly different arguments in a loop.
+
+   In some cases it may make sense to re-run the same set of tests with
+   different options or commands to ensure that the command behaves
+   despite the different parameters. This can be achieved by looping
+   around a specific parameter:
+
+	for arg in '' "--foo"
t/README The "do's:" · line 741
Quoted 10 lines
t/README +10 -0
+	do
+		test_expect_success "test command ${arg:-without arguments}" '
+			command $arg
+		'
+	done
+
+  Note that while the test title uses double quotes ("), the test body
+  should continue to use single quotes ('). The loop variable will be
+  accessible regardless of the single quotes as the test body is passed
+  to `eval`.
We also want to say that they are not equivalent, don't we?
for var in '' a 'b"c'
do
test_expect_success "with dq <$var>" "
echo \"$var\"
"
done

Quoted 7 lines

breaks, but if we use

                test_expect_success "with sq <$var>" '
                        echo "$var"
                '

in the loop, it works as expected.

Hum, good point. How about the below diff? Will reroll the patch series if that looks good to you.

t/README +2 -1

@@ -736,7 +736,8 @@ The "do's:"
        done
 
   Note that while the test title uses double quotes ("), the test body
-  should continue to use single quotes ('). The loop variable will be
+  should continue to use single quotes (') to avoid breakage in case the
+  values contain e.g. quoting characters. The loop variable will be
   accessible regardless of the single quotes as the test body is passed
   to `eval`.

Patrick

Junio C Hamano

2024-03-22 02:12 UTC #6 lore raw

Re: [PATCH 3/3] t/README: document how to loop around test cases

Quoted 6 lines · Patrick Steinhardt

Hum, good point. How about the below diff? Will reroll the patch series if that looks good to you.

t/README The "do's:" · line 742

Quoted 8 lines

t/README +2 -1

@@ -736,7 +736,8 @@ The "do's:"
        done
 
   Note that while the test title uses double quotes ("), the test body
-  should continue to use single quotes ('). The loop variable will be
+  should continue to use single quotes (') to avoid breakage in case the
+  values contain e.g. quoting characters. The loop variable will be
   accessible regardless of the single quotes as the test body is passed
   to `eval`.

Wow, simple and effective.

Patrick Steinhardt

author

2024-03-21 13:48 UTC #7 lore raw

In reply to Patrick Steinhardt

Quoted 7 lines · Patrick Steinhardt

Hi,
SZEDER made me aware [1] of a quoting issue with some test refactorings in t7800 I recently introduced when refactoring the tests to run in a loop. This patch series fixes those issues and also adds a paragraph to "t/README" to document how this is supposed to be done properly.

Quoted 5 lines

Thanks!
Patrick
[1]: https://lore.kernel.org/git/[email protected]/

I forgot to mention: this is built on top of ps/difftool-dir-diff-exit-code at eb84c8b6ce (git-difftool--helper:
honor --trust-exit-code with --dir-diff, 2024-02-20).

Patrick

Patrick Steinhardt

author

2024-03-22 02:23 UTC #8 lore raw

v2 cover t7800: fix quoting of looped test bodies

In reply to Patrick Steinhardt

Hi,

this is the second version of my patch series that aims to address some issues with looping around test_expect_success in t7800.

Changes compared to v1:

- Fixed indentation of a paragraph in "t/README".

- Added a clarification why one wants to use single quotes to "t/README".

Thanks!

Patrick

Patrick Steinhardt (3):
t7800: improve test descriptions with empty arguments
t7800: use single quotes for test bodies t/README: document how to loop around test cases

 t/README            | 20 ++++++++++++++++++++
 t/t7800-difftool.sh | 40 ++++++++++++++++++++--------------------
 2 files changed, 40 insertions(+), 20 deletions(-)

Range-diff against v1:

1:  fd37c29319 = 1:  fd37c29319 t7800: improve test descriptions with empty arguments
2:  a4ca974397 = 2:  a4ca974397 t7800: use single quotes for test bodies
3:  326fb79650 ! 3:  f83b710208 t/README: document how to loop around test cases

@@ t/README: The "do's:"
+ '
+ done
+
-+ Note that while the test title uses double quotes ("), the test body -+ should continue to use single quotes ('). The loop variable will be -+ accessible regardless of the single quotes as the test body is passed -+ to eval.
++ Note that while the test title uses double quotes ("), the test body
++ should continue to use single quotes (') to avoid breakage in case the
++ values contain e.g. quoting characters. The loop variable will be
++ accessible regardless of the single quotes as the test body is passed
++ to eval.
+

And here are the "don'ts:"

Signature · 1 line

2.44.0

Patrick Steinhardt

author

2024-03-22 02:23 UTC #9 lore raw

PATCH v2 1/3 t7800: improve test descriptions with empty arguments

difftool ignores exit code

Improve this by printing "without options" in case the value is empty.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 t/t7800-difftool.sh | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

t/t7800-difftool.sh +8 -8

index 96ae5d5880..80bf108f54 100755
@@ -93,40 +93,40 @@ test_expect_success 'difftool forwards arguments to diff' '
 
 for opt in '' '--dir-diff'
 do
-	test_expect_success "difftool ${opt} ignores exit code" "
+	test_expect_success "difftool ${opt:-without options} ignores exit code" "
 		test_config difftool.error.cmd false &&
 		git difftool ${opt} -y -t error branch
 	"
 
-	test_expect_success "difftool ${opt} forwards exit code with --trust-exit-code" "
+	test_expect_success "difftool ${opt:-without options} forwards exit code with --trust-exit-code" "
 		test_config difftool.error.cmd false &&
 		test_must_fail git difftool ${opt} -y --trust-exit-code -t error branch
 	"
 
-	test_expect_success "difftool ${opt} forwards exit code with --trust-exit-code for built-ins" "
+	test_expect_success "difftool ${opt:-without options} forwards exit code with --trust-exit-code for built-ins" "
 		test_config difftool.vimdiff.path false &&
 		test_must_fail git difftool ${opt} -y --trust-exit-code -t vimdiff branch
 	"
 
-	test_expect_success "difftool ${opt} honors difftool.trustExitCode = true" "
+	test_expect_success "difftool ${opt:-without options} honors difftool.trustExitCode = true" "
 		test_config difftool.error.cmd false &&
 		test_config difftool.trustExitCode true &&
 		test_must_fail git difftool ${opt} -y -t error branch
 	"
 
-	test_expect_success "difftool ${opt} honors difftool.trustExitCode = false" "
+	test_expect_success "difftool ${opt:-without options} honors difftool.trustExitCode = false" "
 		test_config difftool.error.cmd false &&
 		test_config difftool.trustExitCode false &&
 		git difftool ${opt} -y -t error branch
 	"
 
-	test_expect_success "difftool ${opt} ignores exit code with --no-trust-exit-code" "
+	test_expect_success "difftool ${opt:-without options} ignores exit code with --no-trust-exit-code" "
 		test_config difftool.error.cmd false &&
 		test_config difftool.trustExitCode true &&
 		git difftool ${opt} -y --no-trust-exit-code -t error branch
 	"
 
-	test_expect_success "difftool ${opt} stops on error with --trust-exit-code" "
+	test_expect_success "difftool ${opt:-without options} stops on error with --trust-exit-code" "
 		test_when_finished 'rm -f for-diff .git/fail-right-file' &&
 		test_when_finished 'git reset -- for-diff' &&
 		write_script .git/fail-right-file <<-\EOF &&

@@ -140,7 +140,7 @@ do
 		test_line_count = 1 actual
 	"
 
-	test_expect_success "difftool ${opt} honors exit status if command not found" "
+	test_expect_success "difftool ${opt:-without options} honors exit status if command not found" "
 		test_config difftool.nonexistent.cmd i-dont-exist &&
 		test_config difftool.trustExitCode false &&
 		if test "${opt}" = '--dir-diff'

Signature · 1 line

2.44.0

Patrick Steinhardt

author

2024-03-22 02:23 UTC #10 lore raw

PATCH v2 2/3 t7800: use single quotes for test bodies

In reply to Patrick Steinhardt

As the test body is later on passed to eval this change was not required though. Let's revert it back to use single quotes as usual in our tests.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 t/t7800-difftool.sh | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

t/t7800-difftool.sh +20 ~~-20~~

index 80bf108f54..cc917b257e 100755
@@ -93,42 +93,42 @@ test_expect_success 'difftool forwards arguments to diff' '
 
 for opt in '' '--dir-diff'
 do
-	test_expect_success "difftool ${opt:-without options} ignores exit code" "
+	test_expect_success "difftool ${opt:-without options} ignores exit code" '
 		test_config difftool.error.cmd false &&
 		git difftool ${opt} -y -t error branch
-	"
+	'
 
-	test_expect_success "difftool ${opt:-without options} forwards exit code with --trust-exit-code" "
+	test_expect_success "difftool ${opt:-without options} forwards exit code with --trust-exit-code" '
 		test_config difftool.error.cmd false &&
 		test_must_fail git difftool ${opt} -y --trust-exit-code -t error branch
-	"
+	'
 
-	test_expect_success "difftool ${opt:-without options} forwards exit code with --trust-exit-code for built-ins" "
+	test_expect_success "difftool ${opt:-without options} forwards exit code with --trust-exit-code for built-ins" '
 		test_config difftool.vimdiff.path false &&
 		test_must_fail git difftool ${opt} -y --trust-exit-code -t vimdiff branch
-	"
+	'
 
-	test_expect_success "difftool ${opt:-without options} honors difftool.trustExitCode = true" "
+	test_expect_success "difftool ${opt:-without options} honors difftool.trustExitCode = true" '
 		test_config difftool.error.cmd false &&
 		test_config difftool.trustExitCode true &&
 		test_must_fail git difftool ${opt} -y -t error branch
-	"
+	'
 
-	test_expect_success "difftool ${opt:-without options} honors difftool.trustExitCode = false" "
+	test_expect_success "difftool ${opt:-without options} honors difftool.trustExitCode = false" '
 		test_config difftool.error.cmd false &&
 		test_config difftool.trustExitCode false &&
 		git difftool ${opt} -y -t error branch
-	"
+	'
 
-	test_expect_success "difftool ${opt:-without options} ignores exit code with --no-trust-exit-code" "
+	test_expect_success "difftool ${opt:-without options} ignores exit code with --no-trust-exit-code" '
 		test_config difftool.error.cmd false &&
 		test_config difftool.trustExitCode true &&
 		git difftool ${opt} -y --no-trust-exit-code -t error branch
-	"
+	'
 
-	test_expect_success "difftool ${opt:-without options} stops on error with --trust-exit-code" "
-		test_when_finished 'rm -f for-diff .git/fail-right-file' &&
-		test_when_finished 'git reset -- for-diff' &&
+	test_expect_success "difftool ${opt:-without options} stops on error with --trust-exit-code" '
+		test_when_finished "rm -f for-diff .git/fail-right-file" &&
+		test_when_finished "git reset -- for-diff" &&
 		write_script .git/fail-right-file <<-\EOF &&
 		echo failed
 		exit 1

@@ -138,19 +138,19 @@ do
 		test_must_fail git difftool ${opt} -y --trust-exit-code \
 			--extcmd .git/fail-right-file branch >actual &&
 		test_line_count = 1 actual
-	"
+	'
 
-	test_expect_success "difftool ${opt:-without options} honors exit status if command not found" "
+	test_expect_success "difftool ${opt:-without options} honors exit status if command not found" '
 		test_config difftool.nonexistent.cmd i-dont-exist &&
 		test_config difftool.trustExitCode false &&
-		if test "${opt}" = '--dir-diff'
+		if test "${opt}" = --dir-diff
 		then
 			expected_code=127
 		else
 			expected_code=128
 		fi &&
-		test_expect_code \${expected_code} git difftool ${opt} -y -t nonexistent branch
-	"
+		test_expect_code ${expected_code} git difftool ${opt} -y -t nonexistent branch
+	'
 done
 
 test_expect_success 'difftool honors --gui' '

Signature · 1 line

2.44.0

Patrick Steinhardt

author

2024-03-22 02:23 UTC #11 lore raw

PATCH v2 3/3 t/README: document how to loop around test cases

In reply to Patrick Steinhardt

Document the proper way to do this in "t/README".

Signed-off-by Patrick Steinhardt <[email protected]>

---
 t/README | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

t/README +20 -0

index 36463d0742..4422ab9b98 100644
@@ -721,6 +721,26 @@ The "do's:"
    Note that we still &&-chain the loop to propagate failures from
    earlier commands.
 
+ - Repeat tests with slightly different arguments in a loop.
+
+   In some cases it may make sense to re-run the same set of tests with
+   different options or commands to ensure that the command behaves
+   despite the different parameters. This can be achieved by looping
+   around a specific parameter:
+
+	for arg in '' "--foo"
+	do
+		test_expect_success "test command ${arg:-without arguments}" '
+			command $arg
+		'
+	done
+
+   Note that while the test title uses double quotes ("), the test body
+   should continue to use single quotes (') to avoid breakage in case the
+   values contain e.g. quoting characters. The loop variable will be
+   accessible regardless of the single quotes as the test body is passed
+   to `eval`.
+
 
 And here are the "don'ts:"

Signature · 1 line

2.44.0

Junio C Hamano

2024-03-22 14:37 UTC #12 lore raw

In reply to Patrick Steinhardt

Quoted 4 lines · Patrick Steinhardt

Hi,
this is the second version of my patch series that aims to address some issues with looping around test_expect_success in t7800.

Looking good. Thanks. Will queue.

Let's mark it for 'next' already ;-)

Quoted 41 lines

Changes compared to v1:
- Fixed indentation of a paragraph in "t/README".
- Added a clarification why one wants to use single quotes to "t/README".
Thanks!
Patrick
Patrick Steinhardt (3):
t7800: improve test descriptions with empty arguments
t7800: use single quotes for test bodies t/README: document how to loop around test cases
 t/README            | 20 ++++++++++++++++++++
 t/t7800-difftool.sh | 40 ++++++++++++++++++++--------------------
 2 files changed, 40 insertions(+), 20 deletions(-)
Range-diff against v1:
1:  fd37c29319 = 1:  fd37c29319 t7800: improve test descriptions with empty arguments
2:  a4ca974397 = 2:  a4ca974397 t7800: use single quotes for test bodies
3:  326fb79650 ! 3:  f83b710208 t/README: document how to loop around test cases
@@ t/README: The "do's:"
+ '
+ done
+
-+ Note that while the test title uses double quotes ("), the test body -+ should continue to use single quotes ('). The loop variable will be -+ accessible regardless of the single quotes as the test body is passed -+ to eval.
++ Note that while the test title uses double quotes ("), the test body
++ should continue to use single quotes (') to avoid breakage in case the
++ values contain e.g. quoting characters. The loop variable will be
++ accessible regardless of the single quotes as the test body is passed
++ to eval.
+
And here are the "don'ts:"