diff mbox series

[RFC,2/6] t4041, t4060: modernize test style

Message ID 20230213182134.2173280-3-calvinwan@google.com (mailing list archive)
State Superseded
Headers show
Series add: block invalid submodules | expand

Commit Message

Calvin Wan Feb. 13, 2023, 6:21 p.m. UTC 
From: Josh Steadmon <steadmon@google.com>

In preparation for later changes, move setup code into test_expect
blocks. Smaller sections are moved into existing blocks, while larger
sections with a standalone purpose are given their own new blocks.

This makes sure that later changes that may break the test setup are
easier to diagnose, because errors will caught immediately rather than
in later unrelated test_expect blocks.

Signed-off-by: Josh Steadmon <steadmon@google.com>
Signed-off-by: Calvin Wan <calvinwan@google.com>
---
 t/t4041-diff-submodule-option.sh             | 75 +++++++--------
 t/t4060-diff-submodule-option-diff-format.sh | 99 ++++++++++----------
 2 files changed, 87 insertions(+), 87 deletions(-)

Comments

Junio C Hamano Feb. 13, 2023, 7:41 p.m. UTC | #1 
Calvin Wan <calvinwan@google.com> writes:

> From: Josh Steadmon <steadmon@google.com>
>
> In preparation for later changes, move setup code into test_expect
> blocks. Smaller sections are moved into existing blocks, while larger
> sections with a standalone purpose are given their own new blocks.

Nice.

> -test_create_repo sm1 &&
> -add_file . foo >/dev/null
> -
> -head1=$(add_file sm1 foo1 foo2)
> -fullhead1=$(cd sm1; git rev-parse --verify HEAD)
> +test_expect_success 'setup' '
> +	test_create_repo sm1 &&
> +	add_file . foo >/dev/null &&

Now this is inside test_expect_success, redirection to /dev/null is
unnecessary.

> +	head1=$(add_file sm1 foo1 foo2) &&
> +	fullhead1=$(cd sm1 && git rev-parse --verify HEAD)
> +'

Or "fullhead1=$(git -C sm1 rev-parse ...)".

Both of the above can be ignored if we are trying to be a strict
rewrite of the original, but moving code inside test_expect_success
block is a large enough change that there may not be much point in
avoiding such an obvious modernization "while at it".


> -rm sm2
> -mv sm2-bak sm2
> -
>  test_expect_success 'setup nested submodule' '
> +	rm sm2 &&
> +	mv sm2-bak sm2 &&

To me, this looks more like something test_when_finished in the test
that wanted not to have sm2 (i.e. "deleted submodule with .git file")
should have done as part of its own clean-up.

There certainly can be two schools of thought when it comes to how to
arrange the precondition of subsequent tests.  More modern tests tend
to clean after themselves by reverting the damage they made to the
environment inside test_when_finished in themselves.  This one, as
the posted patch does, goes to the other extreme and forces the
subsequent test to undo the damage done by the previous ones.

The latter approach has two major downsides.  It would not work if
the tester wants to skip an earlier step, or if an earlier step
failed to cause the expected damage this step wants to undo.  The
correctness of "what we should see as sm2 here must be in sm2-bak
because we know an earlier step should have moved it there" can
easily be broken.  It also makes it harder to update the earlier
step to cause different damage to the environment---the "undoing the
damage done by the previous step(s)" done as early parts of this
step also needs to be updated.

Whichever approach we pick to use in each script, it would be better
to stick to one philosophy, and if we can make each step revert the
damage it caused when it is done, that would be nice.

> -mv sm2-bak sm2
> +test_expect_success 'submodule cleanup' '
> +	mv sm2-bak sm2
> +'
>  
>  test_done

Likewise.

Thanks.
Calvin Wan Feb. 14, 2023, 8:22 p.m. UTC | #2 
> > -test_create_repo sm1 &&
> > -add_file . foo >/dev/null
> > -
> > -head1=$(add_file sm1 foo1 foo2)
> > -fullhead1=$(cd sm1; git rev-parse --verify HEAD)
> > +test_expect_success 'setup' '
> > +     test_create_repo sm1 &&
> > +     add_file . foo >/dev/null &&
>
> Now this is inside test_expect_success, redirection to /dev/null is
> unnecessary.

ack.

>
> > +     head1=$(add_file sm1 foo1 foo2) &&
> > +     fullhead1=$(cd sm1 && git rev-parse --verify HEAD)
> > +'
>
> Or "fullhead1=$(git -C sm1 rev-parse ...)".
>
> Both of the above can be ignored if we are trying to be a strict
> rewrite of the original, but moving code inside test_expect_success
> block is a large enough change that there may not be much point in
> avoiding such an obvious modernization "while at it".

I agree, will fix.

>
> > -rm sm2
> > -mv sm2-bak sm2
> > -
> >  test_expect_success 'setup nested submodule' '
> > +     rm sm2 &&
> > +     mv sm2-bak sm2 &&
>
> To me, this looks more like something test_when_finished in the test
> that wanted not to have sm2 (i.e. "deleted submodule with .git file")
> should have done as part of its own clean-up.
>
> There certainly can be two schools of thought when it comes to how to
> arrange the precondition of subsequent tests.  More modern tests tend
> to clean after themselves by reverting the damage they made to the
> environment inside test_when_finished in themselves.  This one, as
> the posted patch does, goes to the other extreme and forces the
> subsequent test to undo the damage done by the previous ones.
>
> The latter approach has two major downsides.  It would not work if
> the tester wants to skip an earlier step, or if an earlier step
> failed to cause the expected damage this step wants to undo.  The
> correctness of "what we should see as sm2 here must be in sm2-bak
> because we know an earlier step should have moved it there" can
> easily be broken.  It also makes it harder to update the earlier
> step to cause different damage to the environment---the "undoing the
> damage done by the previous step(s)" done as early parts of this
> step also needs to be updated.
>
> Whichever approach we pick to use in each script, it would be better
> to stick to one philosophy, and if we can make each step revert the
> damage it caused when it is done, that would be nice.
>
> > -mv sm2-bak sm2
> > +test_expect_success 'submodule cleanup' '
> > +     mv sm2-bak sm2
> > +'
> >
> >  test_done
>
> Likewise.

ack. Swapping to test_when_finished
diff mbox series

Patch

diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh
index 0c1502d4b0..556682b18b 100755
--- a/t/t4041-diff-submodule-option.sh
+++ b/t/t4041-diff-submodule-option.sh
@@ -42,11 +42,12 @@  commit_file () {
 	git commit "$@" -m "Commit $*" >/dev/null
 }
 
-test_create_repo sm1 &&
-add_file . foo >/dev/null
-
-head1=$(add_file sm1 foo1 foo2)
-fullhead1=$(cd sm1; git rev-parse --verify HEAD)
+test_expect_success 'setup' '
+	test_create_repo sm1 &&
+	add_file . foo >/dev/null &&
+	head1=$(add_file sm1 foo1 foo2) &&
+	fullhead1=$(cd sm1 && git rev-parse --verify HEAD)
+'
 
 test_expect_success 'added submodule' '
 	git add sm1 &&
@@ -99,10 +100,9 @@  test_expect_success 'diff.submodule does not affect plumbing' '
 	test_cmp expected actual
 '
 
-commit_file sm1 &&
-head2=$(add_file sm1 foo3)
-
 test_expect_success 'modified submodule(forward)' '
+	commit_file sm1 &&
+	head2=$(add_file sm1 foo3) &&
 	git diff-index -p --submodule=log HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 $head1..$head2:
@@ -129,8 +129,8 @@  test_expect_success 'modified submodule(forward) --submodule' '
 	test_cmp expected actual
 '
 
-fullhead2=$(cd sm1; git rev-parse --verify HEAD)
 test_expect_success 'modified submodule(forward) --submodule=short' '
+	fullhead2=$(cd sm1 && git rev-parse --verify HEAD) &&
 	git diff --submodule=short >actual &&
 	cat >expected <<-EOF &&
 	diff --git a/sm1 b/sm1
@@ -144,14 +144,14 @@  test_expect_success 'modified submodule(forward) --submodule=short' '
 	test_cmp expected actual
 '
 
-commit_file sm1 &&
-head3=$(
-	cd sm1 &&
-	git reset --hard HEAD~2 >/dev/null &&
-	git rev-parse --short --verify HEAD
-)
 
 test_expect_success 'modified submodule(backward)' '
+	commit_file sm1 &&
+	head3=$(
+		cd sm1 &&
+		git reset --hard HEAD~2 >/dev/null &&
+		git rev-parse --short --verify HEAD
+	) &&
 	git diff-index -p --submodule=log HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 $head2..$head3 (rewind):
@@ -161,8 +161,8 @@  test_expect_success 'modified submodule(backward)' '
 	test_cmp expected actual
 '
 
-head4=$(add_file sm1 foo4 foo5)
 test_expect_success 'modified submodule(backward and forward)' '
+	head4=$(add_file sm1 foo4 foo5) &&
 	git diff-index -p --submodule=log HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 $head2...$head4:
@@ -174,13 +174,15 @@  test_expect_success 'modified submodule(backward and forward)' '
 	test_cmp expected actual
 '
 
-commit_file sm1 &&
-mv sm1 sm1-bak &&
-echo sm1 >sm1 &&
-head5=$(git hash-object sm1 | cut -c1-7) &&
-git add sm1 &&
-rm -f sm1 &&
-mv sm1-bak sm1
+test_expect_success 'setup - change sm1 to a blob' '
+	commit_file sm1 &&
+	mv sm1 sm1-bak &&
+	echo sm1 >sm1 &&
+	head5=$(git hash-object sm1 | cut -c1-7) &&
+	git add sm1 &&
+	rm -f sm1 &&
+	mv sm1-bak sm1
+'
 
 test_expect_success 'typechanged submodule(submodule->blob), --cached' '
 	git diff --submodule=log --cached >actual &&
@@ -212,9 +214,9 @@  test_expect_success 'typechanged submodule(submodule->blob)' '
 	test_cmp expected actual
 '
 
-rm -rf sm1 &&
-git checkout-index sm1
 test_expect_success 'typechanged submodule(submodule->blob)' '
+	rm -rf sm1 &&
+	git checkout-index sm1 &&
 	git diff-index -p --submodule=log HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 $head4...0000000 (submodule deleted)
@@ -229,11 +231,11 @@  test_expect_success 'typechanged submodule(submodule->blob)' '
 	test_cmp expected actual
 '
 
-rm -f sm1 &&
-test_create_repo sm1 &&
-head6=$(add_file sm1 foo6 foo7)
-fullhead6=$(cd sm1; git rev-parse --verify HEAD)
 test_expect_success 'nonexistent commit' '
+	rm -f sm1 &&
+	test_create_repo sm1 &&
+	head6=$(add_file sm1 foo6 foo7) &&
+	fullhead6=$(cd sm1 && git rev-parse --verify HEAD) &&
 	git diff-index -p --submodule=log HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 $head4...$head6 (commits not present)
@@ -241,8 +243,8 @@  test_expect_success 'nonexistent commit' '
 	test_cmp expected actual
 '
 
-commit_file
 test_expect_success 'typechanged submodule(blob->submodule)' '
+	commit_file &&
 	git diff-index -p --submodule=log HEAD >actual &&
 	cat >expected <<-EOF &&
 	diff --git a/sm1 b/sm1
@@ -257,8 +259,8 @@  test_expect_success 'typechanged submodule(blob->submodule)' '
 	test_cmp expected actual
 '
 
-commit_file sm1 &&
 test_expect_success 'submodule is up to date' '
+	commit_file sm1 &&
 	git diff-index -p --submodule=log HEAD >actual &&
 	test_must_be_empty actual
 '
@@ -327,9 +329,9 @@  test_expect_success 'submodule contains modified content' '
 	test_cmp expected actual
 '
 
-(cd sm1; git commit -mchange foo6 >/dev/null) &&
-head8=$(cd sm1; git rev-parse --short --verify HEAD) &&
 test_expect_success 'submodule is modified' '
+	(cd sm1 && git commit -mchange foo6 >/dev/null) &&
+	head8=$(cd sm1 && git rev-parse --short --verify HEAD) &&
 	git diff-index -p --submodule=log HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 $head6..$head8:
@@ -422,8 +424,8 @@  test_expect_success 'modified submodule contains modified content' '
 	test_cmp expected actual
 '
 
-rm -rf sm1
 test_expect_success 'deleted submodule' '
+	rm -rf sm1 &&
 	git diff-index -p --submodule=log HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 $head6...0000000 (submodule deleted)
@@ -454,8 +456,8 @@  test_expect_success 'path filter' '
 	test_cmp expected actual
 '
 
-commit_file sm2
 test_expect_success 'given commit' '
+	commit_file sm2 &&
 	git diff-index -p --submodule=log HEAD^ >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 $head6...0000000 (submodule deleted)
@@ -473,9 +475,8 @@  test_expect_success 'given commit --submodule' '
 	test_cmp expected actual
 '
 
-fullhead7=$(cd sm2; git rev-parse --verify HEAD)
-
 test_expect_success 'given commit --submodule=short' '
+	fullhead7=$(cd sm2 && git rev-parse --verify HEAD) &&
 	git diff-index -p --submodule=short HEAD^ >actual &&
 	cat >expected <<-EOF &&
 	diff --git a/sm1 b/sm1
diff --git a/t/t4060-diff-submodule-option-diff-format.sh b/t/t4060-diff-submodule-option-diff-format.sh
index 97c6424cd5..3cda8ffd14 100755
--- a/t/t4060-diff-submodule-option-diff-format.sh
+++ b/t/t4060-diff-submodule-option-diff-format.sh
@@ -138,10 +138,9 @@  test_expect_success 'diff.submodule does not affect plumbing' '
 	diff_cmp expected actual
 '
 
-commit_file sm1 &&
-head2=$(add_file sm1 foo3)
-
 test_expect_success 'modified submodule(forward)' '
+	commit_file sm1 &&
+	head2=$(add_file sm1 foo3) &&
 	git diff-index -p --submodule=diff HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 $head1..$head2:
@@ -180,8 +179,8 @@  test_expect_success 'modified submodule(forward) --submodule' '
 	diff_cmp expected actual
 '
 
-fullhead2=$(cd sm1; git rev-parse --verify HEAD)
 test_expect_success 'modified submodule(forward) --submodule=short' '
+	fullhead2=$(cd sm1 && git rev-parse --verify HEAD) &&
 	git diff --submodule=short >actual &&
 	cat >expected <<-EOF &&
 	diff --git a/sm1 b/sm1
@@ -195,14 +194,13 @@  test_expect_success 'modified submodule(forward) --submodule=short' '
 	diff_cmp expected actual
 '
 
-commit_file sm1 &&
-head3=$(
-	cd sm1 &&
-	git reset --hard HEAD~2 >/dev/null &&
-	git rev-parse --short --verify HEAD
-)
-
 test_expect_success 'modified submodule(backward)' '
+	commit_file sm1 &&
+	head3=$(
+		cd sm1 &&
+		git reset --hard HEAD~2 >/dev/null &&
+		git rev-parse --short --verify HEAD
+	) &&
 	git diff-index -p --submodule=diff HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 $head2..$head3 (rewind):
@@ -224,8 +222,8 @@  test_expect_success 'modified submodule(backward)' '
 	diff_cmp expected actual
 '
 
-head4=$(add_file sm1 foo4 foo5)
 test_expect_success 'modified submodule(backward and forward)' '
+	head4=$(add_file sm1 foo4 foo5) &&
 	git diff-index -p --submodule=diff HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 $head2...$head4:
@@ -261,13 +259,15 @@  test_expect_success 'modified submodule(backward and forward)' '
 	diff_cmp expected actual
 '
 
-commit_file sm1 &&
-mv sm1 sm1-bak &&
-echo sm1 >sm1 &&
-head5=$(git hash-object sm1 | cut -c1-7) &&
-git add sm1 &&
-rm -f sm1 &&
-mv sm1-bak sm1
+test_expect_success 'setup - change sm1 to a blob' '
+	commit_file sm1 &&
+	mv sm1 sm1-bak &&
+	echo sm1 >sm1 &&
+	head5=$(git hash-object sm1 | cut -c1-7) &&
+	git add sm1 &&
+	rm -f sm1 &&
+	mv sm1-bak sm1
+'
 
 test_expect_success 'typechanged submodule(submodule->blob), --cached' '
 	git diff --submodule=diff --cached >actual &&
@@ -341,9 +341,9 @@  test_expect_success 'typechanged submodule(submodule->blob)' '
 	diff_cmp expected actual
 '
 
-rm -rf sm1 &&
-git checkout-index sm1
 test_expect_success 'typechanged submodule(submodule->blob)' '
+	rm -rf sm1 &&
+	git checkout-index sm1 &&
 	git diff-index -p --submodule=diff HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 $head4...0000000 (submodule deleted)
@@ -358,10 +358,10 @@  test_expect_success 'typechanged submodule(submodule->blob)' '
 	diff_cmp expected actual
 '
 
-rm -f sm1 &&
-test_create_repo sm1 &&
-head6=$(add_file sm1 foo6 foo7)
 test_expect_success 'nonexistent commit' '
+	rm -f sm1 &&
+	test_create_repo sm1 &&
+	head6=$(add_file sm1 foo6 foo7) &&
 	git diff-index -p --submodule=diff HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 $head4...$head6 (commits not present)
@@ -369,8 +369,8 @@  test_expect_success 'nonexistent commit' '
 	diff_cmp expected actual
 '
 
-commit_file
 test_expect_success 'typechanged submodule(blob->submodule)' '
+	commit_file &&
 	git diff-index -p --submodule=diff HEAD >actual &&
 	cat >expected <<-EOF &&
 	diff --git a/sm1 b/sm1
@@ -399,8 +399,8 @@  test_expect_success 'typechanged submodule(blob->submodule)' '
 	diff_cmp expected actual
 '
 
-commit_file sm1 &&
 test_expect_success 'submodule is up to date' '
+	commit_file sm1 &&
 	head7=$(git -C sm1 rev-parse --short --verify HEAD) &&
 	git diff-index -p --submodule=diff HEAD >actual &&
 	test_must_be_empty actual
@@ -492,9 +492,9 @@  test_expect_success 'submodule contains modified content' '
 	diff_cmp expected actual
 '
 
-(cd sm1; git commit -mchange foo6 >/dev/null) &&
-head8=$(cd sm1; git rev-parse --short --verify HEAD) &&
 test_expect_success 'submodule is modified' '
+	(cd sm1 && git commit -mchange foo6 >/dev/null) &&
+	head8=$(cd sm1 && git rev-parse --short --verify HEAD) &&
 	git diff-index -p --submodule=diff HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 $head7..$head8:
@@ -643,8 +643,8 @@  test_expect_success 'modified submodule contains modified content' '
 	diff_cmp expected actual
 '
 
-rm -rf sm1
 test_expect_success 'deleted submodule' '
+	rm -rf sm1 &&
 	git diff-index -p --submodule=diff HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 $head7...0000000 (submodule deleted)
@@ -703,13 +703,15 @@  test_expect_success 'path filter' '
 	diff_cmp expected actual
 '
 
-cat >.gitmodules <<-EOF
-[submodule "sm2"]
-	path = sm2
-	url = bogus_url
-EOF
-git add .gitmodules
-commit_file sm2 .gitmodules
+test_expect_success 'setup - construct .gitmodules' '
+	cat >.gitmodules <<-EOF &&
+	[submodule "sm2"]
+		path = sm2
+		url = bogus_url
+	EOF
+	git add .gitmodules &&
+	commit_file sm2 .gitmodules
+'
 
 test_expect_success 'given commit' '
 	git diff-index -p --submodule=diff HEAD^ >actual &&
@@ -779,9 +781,8 @@  test_expect_success 'diff --submodule=diff with .git file' '
 	diff_cmp expected actual
 '
 
-mv sm2 sm2-bak
-
 test_expect_success 'deleted submodule with .git file' '
+	mv sm2 sm2-bak &&
 	git diff-index -p --submodule=diff HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 $head7...0000000 (submodule deleted)
@@ -804,9 +805,8 @@  test_expect_success 'deleted submodule with .git file' '
 	diff_cmp expected actual
 '
 
-echo submodule-to-blob>sm2
-
 test_expect_success 'typechanged(submodule->blob) submodule with .git file' '
+	echo submodule-to-blob>sm2 &&
 	git diff-index -p --submodule=diff HEAD >actual &&
 	cat >expected <<-EOF &&
 	Submodule sm1 $head7...0000000 (submodule deleted)
@@ -836,10 +836,9 @@  test_expect_success 'typechanged(submodule->blob) submodule with .git file' '
 	diff_cmp expected actual
 '
 
-rm sm2
-mv sm2-bak sm2
-
 test_expect_success 'setup nested submodule' '
+	rm sm2 &&
+	mv sm2-bak sm2 &&
 	git -c protocol.file.allow=always -C sm2 submodule add ../sm2 nested &&
 	git -C sm2 commit -a -m "nested sub" &&
 	head10=$(git -C sm2 rev-parse --short --verify HEAD)
@@ -910,13 +909,11 @@  test_expect_success 'diff --submodule=diff recurses into nested submodules' '
 	diff_cmp expected actual
 '
 
-(cd sm2; commit_file nested)
-commit_file sm2
-head12=$(cd sm2; git rev-parse --short --verify HEAD)
-
-mv sm2 sm2-bak
-
 test_expect_success 'diff --submodule=diff recurses into deleted nested submodules' '
+	(cd sm2 && commit_file nested) &&
+	commit_file sm2 &&
+	head12=$(cd sm2 && git rev-parse --short --verify HEAD) &&
+	mv sm2 sm2-bak &&
 	cat >expected <<-EOF &&
 	Submodule sm1 $head7...0000000 (submodule deleted)
 	Submodule sm2 $head12...0000000 (submodule deleted)
@@ -971,6 +968,8 @@  test_expect_success 'diff --submodule=diff recurses into deleted nested submodul
 	diff_cmp expected actual
 '
 
-mv sm2-bak sm2
+test_expect_success 'submodule cleanup' '
+	mv sm2-bak sm2
+'
 
 test_done