Cleanup rebase signoff tests
cover Cleanup rebase signoff tests
This series cleans up the tests for "git rebase --signoff" in preparation for extending them and fixing a couple of bugs in a future series. The cleanups are:
* move test setup into "test_expect_success"
* stop running git upstream of a pipe
* restore "git rebase --apply --signoff" coverage
Phillip Wood (3):
t3428: modernize test setup
t3428: use test_commit_message
t3428: restore coverage for "apply" backend
t/t3428-rebase-signoff.sh | 67 ++++++++++++++++++--------------------- 1 file changed, 30 insertions(+), 37 deletions(-)
base-commit: 19981daefd7c147444462739375462b49412ce33
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1713%2Fphillipwood%2Fcleanup-rebase-signoff-tests-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1713/phillipwood/cleanup-rebase-signoff-tests-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1713
Signature · 1 line
gitgitgadget
PATCH 1/3 t3428: modernize test setup
From: Phillip Wood <[email protected]>
Perform the setup in a dedicated test so the later tests can be run independently. Also avoid running git upstream of a pipe and take advantage of test_commit.
--- t/t3428-rebase-signoff.sh | 42 +++++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 21 deletions(-)
t/t3428-rebase-signoff.sh +21 -21
@@ -8,37 +8,37 @@ This test runs git rebase --signoff and make sure that it works. TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh -# A simple file to commit -cat >file <<EOF -a -EOF +test_expect_success 'setup' ' + git commit --allow-empty -m "Initial empty commit" && + test_commit first file a && + + ident="$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" && -# Expected commit message for initial commit after rebase --signoff -cat >expected-initial-signed <<EOF -Initial empty commit + # Expected commit message for initial commit after rebase --signoff + cat >expected-initial-signed <<-EOF && + Initial empty commit -Signed-off-by: $(git var GIT_COMMITTER_IDENT | sed -e "s/>.*/>/") -EOF + Signed-off-by: $ident + EOF -# Expected commit message after rebase --signoff -cat >expected-signed <<EOF -first + # Expected commit message after rebase --signoff + cat >expected-signed <<-EOF && + first -Signed-off-by: $(git var GIT_COMMITTER_IDENT | sed -e "s/>.*/>/") -EOF + Signed-off-by: $ident + EOF -# Expected commit message after rebase without --signoff (or with --no-signoff) -cat >expected-unsigned <<EOF -first -EOF + # Expected commit message after rebase without --signoff (or with --no-signoff) + cat >expected-unsigned <<-EOF && + first + EOF + git config alias.rbs "rebase --signoff" +' # We configure an alias to do the rebase --signoff so that # on the next subtest we can show that --no-signoff overrides the alias test_expect_success 'rebase --signoff adds a sign-off line' ' - git commit --allow-empty -m "Initial empty commit" && - git add file && git commit -m first && - git config alias.rbs "rebase --signoff" && git rbs HEAD^ && git cat-file commit HEAD | sed -e "1,/^\$/d" > actual && test_cmp expected-signed actual
Signature · 1 line
gitgitgadget
PATCH 2/3 t3428: use test_commit_message
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
Using a helper function makes the tests shorter and avoids running "git cat-file" upstream of a pipe.
--- t/t3428-rebase-signoff.sh | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-)
t/t3428-rebase-signoff.sh +7 -14
@@ -40,15 +40,13 @@ test_expect_success 'setup' ' # on the next subtest we can show that --no-signoff overrides the alias test_expect_success 'rebase --signoff adds a sign-off line' ' git rbs HEAD^ && - git cat-file commit HEAD | sed -e "1,/^\$/d" > actual && - test_cmp expected-signed actual + test_commit_message HEAD expected-signed ' test_expect_success 'rebase --no-signoff does not add a sign-off line' ' git commit --amend -m "first" && git rbs --no-signoff HEAD^ && - git cat-file commit HEAD | sed -e "1,/^\$/d" > actual && - test_cmp expected-unsigned actual + test_commit_message HEAD expected-unsigned ' test_expect_success 'rebase --exec --signoff adds a sign-off line' '
@@ -56,30 +54,25 @@ test_expect_success 'rebase --exec --signoff adds a sign-off line' ' git commit --amend -m "first" && git rebase --exec "touch exec" --signoff HEAD^ && test_path_is_file exec && - git cat-file commit HEAD | sed -e "1,/^\$/d" >actual && - test_cmp expected-signed actual + test_commit_message HEAD expected-signed ' test_expect_success 'rebase --root --signoff adds a sign-off line' ' git commit --amend -m "first" && git rebase --root --keep-empty --signoff && - git cat-file commit HEAD^ | sed -e "1,/^\$/d" >actual && - test_cmp expected-initial-signed actual && - git cat-file commit HEAD | sed -e "1,/^\$/d" >actual && - test_cmp expected-signed actual + test_commit_message HEAD^ expected-initial-signed && + test_commit_message HEAD expected-signed ' test_expect_success 'rebase -i --signoff fails' ' git commit --amend -m "first" && git rebase -i --signoff HEAD^ && - git cat-file commit HEAD | sed -e "1,/^\$/d" >actual && - test_cmp expected-signed actual + test_commit_message HEAD expected-signed ' test_expect_success 'rebase -m --signoff fails' ' git commit --amend -m "first" && git rebase -m --signoff HEAD^ && - git cat-file commit HEAD | sed -e "1,/^\$/d" >actual && - test_cmp expected-signed actual + test_commit_message HEAD expected-signed ' test_done
Signature · 1 line
gitgitgadget
Re: [PATCH 2/3] t3428: use test_commit_message
Quoted 4 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
Using a helper function makes the tests shorter and avoids running "git cat-file" upstream of a pipe.
Nice.
PATCH 3/3 t3428: restore coverage for "apply" backend
In reply to Phillip Wood via GitGitGadgetFrom: Phillip Wood <[email protected]>
This test file assumes the "apply" backend is the default which is not the case since 2ac0d6273f (rebase: change the default backend from "am" to "merge", 2020-02-15). Make sure the "apply" backend is tested by specifying it explicitly.
--- t/t3428-rebase-signoff.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
t/t3428-rebase-signoff.sh +2 -2
@@ -38,8 +38,8 @@ test_expect_success 'setup' ' # We configure an alias to do the rebase --signoff so that # on the next subtest we can show that --no-signoff overrides the alias -test_expect_success 'rebase --signoff adds a sign-off line' ' - git rbs HEAD^ && +test_expect_success 'rebase --apply --signoff adds a sign-off line' ' + git rbs --apply HEAD^ && test_commit_message HEAD expected-signed '
Signature · 1 line
gitgitgadget
Re: [PATCH 3/3] t3428: restore coverage for "apply" backend
Quoted 6 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
This test file assumes the "apply" backend is the default which is not the case since
2ac0d6273f(rebase: change the default backend from "am" to "merge", 2020-02-15). Make sure the "apply" backend is tested by specifying it explicitly.
Hmph, doesn't this lose coverage for the merge backend, though?
Quoted 19 lines
Signed-off-by Phillip Wood <[email protected]>--- t/t3428-rebase-signoff.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)t/t3428-rebase-signoff.sh +2
-2@@ -38,8 +38,8 @@ test_expect_success 'setup' ' # We configure an alias to do the rebase --signoff so that # on the next subtest we can show that --no-signoff overrides the alias -test_expect_success 'rebase --signoff adds a sign-off line' ' - git rbs HEAD^ && +test_expect_success 'rebase --apply --signoff adds a sign-off line' ' + git rbs --apply HEAD^ && test_commit_message HEAD expected-signed '
Re: [PATCH 3/3] t3428: restore coverage for "apply" backend
Hi Junio
Quoted 10 lines · Junio C Hamano
Quoted 6 lines · Phillip Wood via GitGitGadget
From: Phillip Wood <[email protected]>
This test file assumes the "apply" backend is the default which is not the case since
2ac0d6273f(rebase: change the default backend from "am" to "merge", 2020-02-15). Make sure the "apply" backend is tested by specifying it explicitly.Hmph, doesn't this lose coverage for the merge backend, though?
I don't think so, we had coverage for the merge backend from the other tests before 2ac0d6273f as all of the other tests in this file use the merge backend. We're no longer testing "--signoff" without specifying some other option that selects a backend but it seems unlikely that we could break that without breaking one of the other tests.
Best Wishes
Phillip
Quoted 20 lines
Quoted 20 lines
Signed-off-by Phillip Wood <[email protected]>--- t/t3428-rebase-signoff.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)t/t3428-rebase-signoff.sh +2
-2@@ -38,8 +38,8 @@ test_expect_success 'setup' ' # We configure an alias to do the rebase --signoff so that # on the next subtest we can show that --no-signoff overrides the alias -test_expect_success 'rebase --signoff adds a sign-off line' ' - git rbs HEAD^ && +test_expect_success 'rebase --apply --signoff adds a sign-off line' ' + git rbs --apply HEAD^ && test_commit_message HEAD expected-signed '
Re: [PATCH 3/3] t3428: restore coverage for "apply" backend
Quoted 7 lines · [email protected] writes:
Quoted 1 line
Hmph, doesn't this lose coverage for the merge backend, though?
I don't think so, we had coverage for the merge backend from the other tests before
2ac0d6273fas all of the other tests in this file use the merge backend. We're no longer testing "--signoff" without specifying some other option that selects a backend but it seems unlikely that we could break that without breaking one of the other tests.
OK, so we have "rebase --merge --signoff" tested elsewhere and we are replacing "rebase --signoff" with "rebase --apply --signoff"?
Thanks.
Re: [PATCH 3/3] t3428: restore coverage for "apply" backend
Quoted 4 lines · Junio C Hamano
Quoted 2 lines · [email protected] writes:
Quoted 1 line
Hmph, doesn't this lose coverage for the merge backend, though?
Quoted 8 lines
Quoted 5 lines
I don't think so, we had coverage for the merge backend from the other tests before
2ac0d6273fas all of the other tests in this file use the merge backend. We're no longer testing "--signoff" without specifying some other option that selects a backend but it seems unlikely that we could break that without breaking one of the other tests.OK, so we have "rebase --merge --signoff" tested elsewhere and we are replacing "rebase --signoff" with "rebase --apply --signoff"?
Exactly
Quoted 2 lines
Thanks.
Re: [PATCH 3/3] t3428: restore coverage for "apply" backend
Quoted 10 lines · [email protected] writes:
Quoted 9 lines · Junio C Hamano
Quoted 7 lines · [email protected] writes:
Quoted 1 line
Hmph, doesn't this lose coverage for the merge backend, though?
I don't think so, we had coverage for the merge backend from the other tests before
2ac0d6273fas all of the other tests in this file use the merge backend. We're no longer testing "--signoff" without specifying some other option that selects a backend but it seems unlikely that we could break that without breaking one of the other tests.
Quoted 4 lines
Quoted 2 lines
OK, so we have "rebase --merge --signoff" tested elsewhere and we are replacing "rebase --signoff" with "rebase --apply --signoff"?
Exactly
Perhaps we can write that in the log message to help the next person who reads the patch? Something like...
t3428: restore coverage for "apply" backend
This test file assumes the "apply" backend is the default which is not the case since 2ac0d6273f (rebase: change the default backend from "am" to "merge", 2020-02-15). The way "merge" backend honors "--signoff" is already tested elsewhere, so make sure the "apply" backend is tested by specifying it explicitly.
Re: [PATCH 3/3] t3428: restore coverage for "apply" backend
Quoted 22 lines · Junio C Hamano
Quoted 10 lines · [email protected] writes:
Quoted 9 lines · Junio C Hamano
Quoted 7 lines · [email protected] writes:
Quoted 1 line
Hmph, doesn't this lose coverage for the merge backend, though?
I don't think so, we had coverage for the merge backend from the other tests before
2ac0d6273fas all of the other tests in this file use the merge backend. We're no longer testing "--signoff" without specifying some other option that selects a backend but it seems unlikely that we could break that without breaking one of the other tests.Quoted 4 lines
Quoted 2 lines
OK, so we have "rebase --merge --signoff" tested elsewhere and we are replacing "rebase --signoff" with "rebase --apply --signoff"?
Exactly
Perhaps we can write that in the log message to help the next person who reads the patch? Something like...
t3428: restore coverage for "apply" backend
Quoted 8 lines
This test file assumes the "apply" backend is the default which is not the case since
2ac0d6273f(rebase: change the default backend from "am" to "merge", 2020-02-15). The way "merge" backend honors "--signoff" is already tested elsewhere, so make sure the "apply" backend is tested by specifying it explicitly.Signed-off-by Phillip Wood <[email protected]>Signed-off-by Junio C Hamano <[email protected]>
Sounds good, I'll send a re-roll
Thanks
Phillip
Re: [PATCH 3/3] t3428: restore coverage for "apply" backend
Quoted 14 lines · On 12/04/2024 10:33, [email protected] wrote:
Quoted 4 lines · Junio C Hamano
[email protected] writes:
Perhaps we can write that in the log message to help the next person who reads the patch? Something like...Quoted 8 lines
t3428: restore coverage for "apply" backend
This test file assumes the "apply" backend is the default which is not the case since2ac0d6273f(rebase: change the default backend from "am" to "merge", 2020-02-15). The way "merge" backend honors "--signoff" is already tested elsewhere, so make sure the "apply" backend is tested by specifying it explicitly.Signed-off-by Phillip Wood <[email protected]>Signed-off-by Junio C Hamano <[email protected]>
Quoted 1 line
Sounds good, I'll send a re-roll
Oh, it looks like this hit next yesterday
Phillip