Re: [GSoC][PATCH v7 04/26] stash: renamed test cases to be more descriptive

From: Thomas Gummerer <t.gummerer@gmail.com>
Date: Aug 15, 2018, 19:31 UTC
Message-ID: <20180815193133.GJ2734@hank.intra.tgummerer.com>
In-Reply-To: <cb70dc2317b8bdbe640a50de7050cb405067c0f0.1533753605.git.ungureanupaulsebastian@gmail.com>

> Subject: Re: [GSoC][PATCH v7 04/26] stash: renamed test cases to be more descriptive

Please use the imperative mood in the title and the commit messages themselves. From Documentation/SubmittingPatches:

    Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
    instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
    to do frotz", as if you are giving orders to the codebase to change
    its behavior.

From a quick skim over the rest of the series, this also applies to some of the subsequent patches in the series.

On 08/08, Paul-Sebastian Ungureanu wrote:

Show 36 quoted lines

> Renamed some test cases' labels to be more descriptive and under 80
> characters per line.
> 
> Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>
> ---
>  t/t3903-stash.sh | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index de6cab1fe..8d002a7f2 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -604,7 +604,7 @@ test_expect_success 'stash show -p - no stashes on stack, stash-like argument' '
>  	test_cmp expected actual
>  '
>  
> -test_expect_success 'stash drop - fail early if specified stash is not a stash reference' '
> +test_expect_success 'drop: fail early if specified stash is not a stash ref' '
>  	git stash clear &&
>  	test_when_finished "git reset --hard HEAD && git stash clear" &&
>  	git reset --hard &&
> @@ -618,7 +618,7 @@ test_expect_success 'stash drop - fail early if specified stash is not a stash r
>  	git reset --hard HEAD
>  '
>  
> -test_expect_success 'stash pop - fail early if specified stash is not a stash reference' '
> +test_expect_success 'pop: fail early if specified stash is not a stash ref' '
>  	git stash clear &&
>  	test_when_finished "git reset --hard HEAD && git stash clear" &&
>  	git reset --hard &&
> @@ -682,7 +682,7 @@ test_expect_success 'invalid ref of the form "n", n >= N' '
>  	git stash drop
>  '
>  
> -test_expect_success 'stash branch should not drop the stash if the branch exists' '
> +test_expect_success 'branch: should not drop the stash if the branch exists' '

Since we're adjusting the titles of the tests here I'll allow myself to nitpick a little :)

Maybe "branch: do not drop the stash if the branch exists", which sounds more like an assertion, as the "pop" and "drop" titles above.

Show 18 quoted lines

>  	git stash clear &&
>  	echo foo >file &&
>  	git add file &&
> @@ -693,7 +693,7 @@ 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' '
> +test_expect_success 'branch: should not drop the stash if the apply fails' '
>  	git stash clear &&
>  	git reset HEAD~1 --hard &&
>  	echo foo >file &&
> @@ -707,7 +707,7 @@ test_expect_success 'stash branch should not drop the stash if the apply fails'
>  	git rev-parse stash@{0} --
>  '
>  
> -test_expect_success 'stash apply shows status same as git status (relative to current directory)' '
> +test_expect_success 'apply: shows same status as git status (relative to ./)' '

s/shows/show/ above maybe? This used to be a full sentence previously, where 'shows' was appropriate, but I think "show" sounds better after the colon.

Show 9 quoted lines

>  	git stash clear &&
>  	echo 1 >subdir/subfile1 &&
>  	echo 2 >subdir/subfile2 &&
> @@ -1048,7 +1048,7 @@ test_expect_success 'stash push -p with pathspec shows no changes only once' '
>  	test_i18ncmp expect actual
>  '
>  
> -test_expect_success 'stash push with pathspec shows no changes when there are none' '
> +test_expect_success 'push: <pathspec> shows no changes when there are none' '

Maybe "push <pathspec>: show no changes when there are none"? "push <pathspec>" would be the rest of the 'git stash' command, having the colon in between them seems a bit odd.

Show 9 quoted lines

>  	>foo &&
>  	git add foo &&
>  	git commit -m "tmp" &&
> @@ -1058,7 +1058,7 @@ test_expect_success 'stash push with pathspec shows no changes when there are no
>  	test_i18ncmp expect actual
>  '
>  
> -test_expect_success 'stash push with pathspec not in the repository errors out' '
> +test_expect_success 'push: <pathspec> not in the repository errors out' '

This one makes sense to me.

Show 6 quoted lines

>  	>untracked &&
>  	test_must_fail git stash push untracked &&
>  	test_path_is_file untracked
> -- 
> 2.18.0.573.g56500d98f
>