diff mbox series

[v3,2/2,Outreachy,v1] t3404: employing test_line_count() to replace test

Message ID 37b1411ee2c420f1a8578d27a2f7d54ccd3f329f.1728230769.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series t3404: avoid losing exit status to pipes | expand

Commit Message

Usman Akinyemi Oct. 6, 2024, 4:06 p.m. UTC 
From: Usman Akinyemi <usmanakinyemi202@gmail.com>

Refactor t3404 to replace instances of `test` with `test_line_count()`
for checking line counts. This improves readability and aligns with Git's
current test practices.

Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
 t/t3404-rebase-interactive.sh | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

Comments

Kristoffer Haugsbakk Oct. 6, 2024, 4:31 p.m. UTC | #1 
On Sun, Oct 6, 2024, at 18:06, Usman Akinyemi via GitGitGadget wrote:
> From: Usman Akinyemi <usmanakinyemi202@gmail.com>
>
> Refactor t3404 to replace instances of `test` with `test_line_count()`
> for checking line counts. This improves readability and aligns with Git's
> current test practices.
>
> Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>

Nice commit message.
Eric Sunshine Oct. 7, 2024, 4:38 a.m. UTC | #2 
On Sun, Oct 6, 2024 at 12:06 PM Usman Akinyemi via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> Refactor t3404 to replace instances of `test` with `test_line_count()`
> for checking line counts. This improves readability and aligns with Git's
> current test practices.
>
> Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
> ---
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> @@ -281,7 +281,8 @@ test_expect_success 'stop on conflicting pick' '
> -       test 4 = $(grep -v "^#" < .git/rebase-merge/done | wc -l) &&
> +       grep -v "^#" < .git/rebase-merge/done > actual &&
> +       test_line_count = 4 actual &&

Thanks. Both patches in this series look fine.

Let's consider this a successfully-completed microproject.
Patrick Steinhardt Oct. 7, 2024, 6:05 a.m. UTC | #3 
On Sun, Oct 06, 2024 at 04:06:09PM +0000, Usman Akinyemi via GitGitGadget wrote:
> @@ -401,8 +402,8 @@ test_expect_success 'multi-squash only fires up editor once' '
>  	) &&
>  	test $base = $(git rev-parse HEAD^) &&
>  	git show >output &&
> -	count=$(grep ONCE output | wc -l) &&
> -	test 1 = $count
> +	grep ONCE output >actual &&
> +	test_line_count = 1 actual
>  '
>  
>  test_expect_success 'multi-fixup does not fire up editor' '

Oh, you already do the change I proposed on the first commit. It's a bit
funny that we first change things one way and then touch it up again in
another commit as it leaves the reviewer wondering for a bit.

But I guess that's okay, especially for a microproject. So overall I
don't see a strong reason to reroll this series, thanks!

Patrick
Usman Akinyemi Oct. 7, 2024, 7:32 a.m. UTC | #4 
On Mon, Oct 7, 2024 at 6:05 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Sun, Oct 06, 2024 at 04:06:09PM +0000, Usman Akinyemi via GitGitGadget wrote:
> > @@ -401,8 +402,8 @@ test_expect_success 'multi-squash only fires up editor once' '
> >       ) &&
> >       test $base = $(git rev-parse HEAD^) &&
> >       git show >output &&
> > -     count=$(grep ONCE output | wc -l) &&
> > -     test 1 = $count
> > +     grep ONCE output >actual &&
> > +     test_line_count = 1 actual
> >  '
> >
> >  test_expect_success 'multi-fixup does not fire up editor' '
>
> Oh, you already do the change I proposed on the first commit. It's a bit
> funny that we first change things one way and then touch it up again in
> another commit as it leaves the reviewer wondering for a bit.
>
> But I guess that's okay, especially for a microproject. So overall I
> don't see a strong reason to reroll this series, thanks!
>
> Patrick

Thank you very much for the review. I really appreciate it. I had to
make this separate commit as recommended in the resources you provided
and reviews from other reviewers.
Phillip Wood Oct. 7, 2024, 9 a.m. UTC | #5 
Hi Patrick

On 07/10/2024 07:05, Patrick Steinhardt wrote:
> 
> Oh, you already do the change I proposed on the first commit. It's a bit
> funny that we first change things one way and then touch it up again in
> another commit as it leaves the reviewer wondering for a bit.
> 
> But I guess that's okay, especially for a microproject. So overall I
> don't see a strong reason to reroll this series, thanks!

It looks like Eric suggested making this change in a separate patch. 
While they could perhaps be made in the same patch fixing the pipelines 
and using test_line_count are essentially independent changes so I think 
splitting them into two patches is good practice and makes sense from 
pedagogical point of view.

Best Wishes

Phillip
diff mbox series

Patch

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 96a65783c47..19390eaf331 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -281,7 +281,8 @@  test_expect_success 'stop on conflicting pick' '
 	test_cmp expect2 file1 &&
 	test "$(git diff --name-status |
 		sed -n -e "/^U/s/^U[^a-z]*//p")" = file1 &&
-	test 4 = $(grep -v "^#" < .git/rebase-merge/done | wc -l) &&
+	grep -v "^#" < .git/rebase-merge/done > actual &&
+	test_line_count = 4 actual &&
 	test 0 = $(grep -c "^[^#]" < .git/rebase-merge/git-rebase-todo)
 '
 
@@ -401,8 +402,8 @@  test_expect_success 'multi-squash only fires up editor once' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep ONCE output | wc -l) &&
-	test 1 = $count
+	grep ONCE output >actual &&
+	test_line_count = 1 actual
 '
 
 test_expect_success 'multi-fixup does not fire up editor' '
@@ -436,8 +437,8 @@  test_expect_success 'commit message used after conflict' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep ONCE output | wc -l) &&
-	test 1 = $count &&
+	grep ONCE output >actual &&
+	test_line_count = 1 actual &&
 	git checkout @{-1} &&
 	git branch -D conflict-fixup
 '
@@ -456,8 +457,8 @@  test_expect_success 'commit message retained after conflict' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep TWICE output | wc -l) &&
-	test 2 = $count &&
+	grep TWICE output >actual &&
+	test_line_count = 2 actual &&
 	git checkout @{-1} &&
 	git branch -D conflict-squash
 '
@@ -501,8 +502,8 @@  test_expect_success 'squash ignores comments' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep ONCE output | wc -l) &&
-	test 1 = $count &&
+	grep ONCE output >actual &&
+	test_line_count = 1 actual &&
 	git checkout @{-1} &&
 	git branch -D skip-comments
 '
@@ -519,8 +520,8 @@  test_expect_success 'squash ignores blank lines' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep ONCE output | wc -l) &&
-	test 1 = $count &&
+	grep ONCE output >actual &&
+	test_line_count = 1 actual &&
 	git checkout @{-1} &&
 	git branch -D skip-blank-lines
 '