threads / patch / 46629

patch, 3 partsstash: add test for stash create with no files

Subject: [PATCH 1/3] stash: add test for stash create with no files

## tl;dr

6 messages between Aug 19, 2017 and Aug 19, 2017. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

Joel Teichroeb· Aug 19, 2017, 20:13 UTC · lore

Ensure the command suceeds and outputs nothing

Signed-off-by: Joel Teichroeb <joel@teichroeb.net>
---
 t/t3903-stash.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

Show changes to t/t3903-stash.sh +8 −0

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 4046817d70..f0708ced27 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -444,6 +444,14 @@ test_expect_failure 'stash file to directory' '
 	test foo = "$(cat file/file)"
 '
 
+test_expect_success 'stash create - no changes' '
+	git stash clear &&
+	test_when_finished "git reset --hard HEAD" &&
+	git reset --hard &&
+	git stash create >actual &&
+	test_must_be_empty actual
+'
+
 test_expect_success 'stash branch - no stashes on stack, stash-like argument' '
 	git stash clear &&
 	test_when_finished "git reset --hard HEAD" &&

-- 
2.14.1

Joel Teichroeb· Aug 19, 2017, 20:13 UTC · re: Joel Teichroeb · lore

[PATCH 2/3] stash: Add a test for when apply fails during stash branch

If the return value of merge recursive is not checked, the stash could end up being dropped even though it was not applied properly

Signed-off-by: Joel Teichroeb <joel@teichroeb.net>
---
 t/t3903-stash.sh | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Show changes to t/t3903-stash.sh +14 −0

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index f0708ced27..887010c497 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -656,6 +656,20 @@ test_expect_success 'stash branch should not drop the stash if the branch exists
 	git rev-parse stash@{0} --
 '
 
+test_expect_success 'stash branch should not drop the stash if the apply fails' '
+	git stash clear &&
+	git reset HEAD~1 --hard &&
+	echo foo >file &&
+	git add file &&
+	git commit -m initial &&
+	echo bar >file &&
+	git stash &&
+	echo baz >file &&
+	test_when_finished "git checkout master" &&
+	test_must_fail git stash branch new_branch stash@{0} &&
+	git rev-parse stash@{0} --
+'
+
 test_expect_success 'stash apply shows status same as git status (relative to current directory)' '
 	git stash clear &&
 	echo 1 >subdir/subfile1 &&

-- 
2.14.1

Joel Teichroeb· Aug 19, 2017, 20:13 UTC · re: Joel Teichroeb · lore

[PATCH 3/3] stash: add test for stashing in a detached state

All that we are really testing here is that the message is correct when we are not on any branch. All other functionality is already tested elsewhere.

Signed-off-by: Joel Teichroeb <joel@teichroeb.net>
---
 t/t3903-stash.sh | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Show changes to t/t3903-stash.sh +12 −0

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 887010c497..3b1ac1971a 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -822,6 +822,18 @@ test_expect_success 'create with multiple arguments for the message' '
 	test_cmp expect actual
 '
 
+test_expect_success 'create in a detached state' '
+	test_when_finished "git checkout master" &&
+	git checkout HEAD~1 &&
+	>foo &&
+	git add foo &&
+	STASH_ID=$(git stash create) &&
+	HEAD_ID=$(git rev-parse --short HEAD) &&
+	echo "WIP on (no branch): ${HEAD_ID} initial" >expect &&
+	git show --pretty=%s -s ${STASH_ID} >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'stash -- <pathspec> stashes and restores the file' '
 	>foo &&
 	>bar &&

-- 
2.14.1

Junio C Hamano· Aug 19, 2017, 20:55 UTC · re: Joel Teichroeb · lore

Re: [PATCH 1/3] stash: add test for stash create with no files

I see three patches that add tests, but it is hard to judge them without any explanation on what the point of them are.

Are you documenting an existing breakage? Are you extending test coverage for some breakage we recently fixed without adding tests to ensure that the fix will stay unbroken? Are you planning to touch the implementation (perhaps to add yet another feature that uses some existing code) and adding new tests in advance to avoid breaking the existing code? Some other motivation?

These would have made fine material to write in the cover letter for this series.

Thanks.

Joel Teichroeb· Aug 19, 2017, 21:00 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/3] stash: add test for stash create with no files

Hi Junio,

I was just too lazy to write a cover letter, and thought these would make sense on their own. I'll make sure to include a cover letter next time.

I just ripped them out of my patch series on implementing stash as a builtin[1]. Since I haven't had time, I figured I could at least get the additional tests I wrote into the codebase.

The tests mainly expand coverage of git stash, covering a few critical error cases that didn't have tests.

[1] https://public-inbox.org/git/20170608005535.13080-1-joel@teichroeb.net/

On Sat, Aug 19, 2017 at 1:55 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 14 quoted lines

> I see three patches that add tests, but it is hard to judge them
> without any explanation on what the point of them are.
>
> Are you documenting an existing breakage?  Are you extending test
> coverage for some breakage we recently fixed without adding tests to
> ensure that the fix will stay unbroken?  Are you planning to touch
> the implementation (perhaps to add yet another feature that uses
> some existing code) and adding new tests in advance to avoid breaking
> the existing code?  Some other motivation?
>
> These would have made fine material to write in the cover letter for
> this series.
>
> Thanks.

Junio C Hamano· Aug 19, 2017, 21:07 UTC · re: Joel Teichroeb · lore

Re: [PATCH 1/3] stash: add test for stash create with no files

Joel Teichroeb <joel@teichroeb.net> writes:

Show 17 quoted lines

> On Sat, Aug 19, 2017 at 1:55 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Are you documenting an existing breakage?  Are you extending test
>> coverage for some breakage we recently fixed without adding tests to
>> ensure that the fix will stay unbroken?  Are you planning to touch
>> the implementation (perhaps to add yet another feature that uses
>> some existing code) and adding new tests in advance to avoid breaking
>> the existing code?  Some other motivation?
>>
>
> I was just too lazy to write a cover letter, and thought these would
> make sense on their own. I'll make sure to include a cover letter next
> time.
>
> I just ripped them out of my patch series on implementing stash as a
> builtin[1]. Since I haven't had time, I figured I could at least get
> the additional tests I wrote into the codebase.

OK, so it is the third one among my random 4 choices ;-)

They do look sensible; I did not want to make sure they are not duplicates myself (i.e. a new test that makes sense may not be an undesired addition if the only effect it has is to eat more cycles without improving test coverage).

Will queue.  Thanks.

Show 5 quoted lines

> The tests mainly expand coverage of git stash, covering a few critical
> error cases that didn't have tests.
>
> [1] https://public-inbox.org/git/20170608005535.13080-1-joel@teichroeb.net/
>