diff mbox series

[v2,2/4] show-branch tests: modernize test code

Message ID patch-2.4-27f94abaed-20210617T105245Z-avarab@gmail.com (mailing list archive)
State Accepted
Commit 9b6e74a9c0133c4e34f6ec56cc93a6a83eba2c3c
Headers show
Series show-branch: add missing tests, trivial color output change | expand

Commit Message

Ævar Arnfjörð Bjarmason June 17, 2021, 10:53 a.m. UTC
Modernize test code added in ce567d1867a (Add test to show that
show-branch misses out the 8th column, 2008-07-23) and
11ee57bc4c4 (sort_in_topological_order(): avoid setting a commit flag,
2008-07-23) to use test helpers.

I'm renaming "out" to "actual" for consistency with other tests, and
introducing a "branches.sorted" file in the setup, to make it clear
that it's important that the list be sorted in this particular way.

The "show-branch" output is indented with spaces, which would cause
complaints under "git show --check" with an indented here-doc
block. Let's prefix the lines with "> " to work around that, and to
make it clear that the leading whitespace is important.

We can also get rid of the hardcoding of "main" added here in
334afbc76fb (tests: mark tests relying on the current default for
`init.defaultBranch`, 2020-11-18). For this test we're setting up an
"initial" commit anyway, and now that we've moved over to test_commit
we can reference that instead.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t3202-show-branch.sh | 92 ++++++++++++++++++------------------------
 1 file changed, 40 insertions(+), 52 deletions(-)

Comments

Felipe Contreras June 17, 2021, 9:29 p.m. UTC | #1
Ævar Arnfjörð Bjarmason wrote:
> Modernize test code added in ce567d1867a (Add test to show that
> show-branch misses out the 8th column, 2008-07-23) and
> 11ee57bc4c4 (sort_in_topological_order(): avoid setting a commit flag,
> 2008-07-23) to use test helpers.
> 
> I'm renaming "out" to "actual" for consistency with other tests, and
> introducing a "branches.sorted" file in the setup, to make it clear
> that it's important that the list be sorted in this particular way.

That is better.

> The "show-branch" output is indented with spaces, which would cause
> complaints under "git show --check" with an indented here-doc
> block. Let's prefix the lines with "> " to work around that, and to
> make it clear that the leading whitespace is important.

I'm not sure this is an improvement. To me the original code is just
fine. Also, I don't think writing an 'expect' file belong in a setup
step.

Additionally I would do this change in a separate patch.

> We can also get rid of the hardcoding of "main" added here in
> 334afbc76fb (tests: mark tests relying on the current default for
> `init.defaultBranch`, 2020-11-18). For this test we're setting up an
> "initial" commit anyway, and now that we've moved over to test_commit
> we can reference that instead.

That's also good.

All the changes in this patch look good to me, however, they are smashed
together in a way that makes the review harder, I see:

 1. Use test_commit
 2. Rename out to actual
 3. Use >actual instead of > actual
 4. Use test_seq instead of $numbers
 5. Use branches.sorted instead of branch$i
 6. Use test_config instead of git config
 7. Use internal sed 's/^ //' instead of outside cat

I'm on-board with 6 out of 7, but if these were done it at least 2
patches, they would be clearer. I in fact would prefer one patch per
change (although maybe squash 3 with 2).

Cheers.
diff mbox series

Patch

diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh
index 8cfbbf79c1..7b06048905 100755
--- a/t/t3202-show-branch.sh
+++ b/t/t3202-show-branch.sh
@@ -2,69 +2,57 @@ 
 
 test_description='test show-branch'
 
-GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
-export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
-
 . ./test-lib.sh
 
-numbers="1 2 3 4 5 6 7 8 9 10"
-
 test_expect_success 'setup' '
-
-	> file &&
-	git add file &&
-	test_tick &&
-	git commit -m initial &&
-
-	for i in $numbers
+	test_commit initial &&
+	for i in $(test_seq 1 10)
 	do
-		git checkout -b branch$i main &&
-		> file$i &&
-		git add file$i &&
-		test_tick &&
-		git commit -m branch$i || return 1
-	done
-
+		git checkout -b branch$i initial &&
+		test_commit --no-tag branch$i
+	done &&
+	git for-each-ref \
+		--sort=version:refname \
+		--format="%(refname:strip=2)" \
+		"refs/heads/branch*" >branches.sorted &&
+	sed "s/^> //" >expect <<-\EOF
+	> ! [branch1] branch1
+	>  ! [branch2] branch2
+	>   ! [branch3] branch3
+	>    ! [branch4] branch4
+	>     ! [branch5] branch5
+	>      ! [branch6] branch6
+	>       ! [branch7] branch7
+	>        ! [branch8] branch8
+	>         ! [branch9] branch9
+	>          * [branch10] branch10
+	> ----------
+	>          * [branch10] branch10
+	>         +  [branch9] branch9
+	>        +   [branch8] branch8
+	>       +    [branch7] branch7
+	>      +     [branch6] branch6
+	>     +      [branch5] branch5
+	>    +       [branch4] branch4
+	>   +        [branch3] branch3
+	>  +         [branch2] branch2
+	> +          [branch1] branch1
+	> +++++++++* [branch10^] initial
+	EOF
 '
 
-cat > expect << EOF
-! [branch1] branch1
- ! [branch2] branch2
-  ! [branch3] branch3
-   ! [branch4] branch4
-    ! [branch5] branch5
-     ! [branch6] branch6
-      ! [branch7] branch7
-       ! [branch8] branch8
-        ! [branch9] branch9
-         * [branch10] branch10
-----------
-         * [branch10] branch10
-        +  [branch9] branch9
-       +   [branch8] branch8
-      +    [branch7] branch7
-     +     [branch6] branch6
-    +      [branch5] branch5
-   +       [branch4] branch4
-  +        [branch3] branch3
- +         [branch2] branch2
-+          [branch1] branch1
-+++++++++* [branch10^] initial
-EOF
-
 test_expect_success 'show-branch with more than 8 branches' '
-
-	git show-branch $(for i in $numbers; do echo branch$i; done) > out &&
-	test_cmp expect out
-
+	git show-branch $(cat branches.sorted) >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'show-branch with showbranch.default' '
-	for i in $numbers; do
-		git config --add showbranch.default branch$i
+	for branch in $(cat branches.sorted)
+	do
+		test_config showbranch.default $branch --add
 	done &&
-	git show-branch >out &&
-	test_cmp expect out
+	git show-branch >actual &&
+	test_cmp expect actual
 '
 
 test_done