Re: [GSoC][PATCH v7 20/26] stash: add tests for `git stash push -q`

From: Thomas Gummerer <t.gummerer@gmail.com>
Date: Aug 18, 2018, 16:12 UTC
Message-ID: <20180818161257.GD11326@hank.intra.tgummerer.com>
In-Reply-To: <5ec3429340f43caad79e4213a7994738cb5d33c7.1533753605.git.ungureanupaulsebastian@gmail.com>

On 08/08, Paul-Sebastian Ungureanu wrote:

> This commit introduces more tests for the quiet option of
> `git stash push`.

I think this commit should be squashed into the previous one, so we have implementation and tests in one commit. That way it's easier to see during review that there are tests for the change. For more discussion on that also see [1].

[1]: https://public-inbox.org/git/20180806144726.GB97564@aiede.svl.corp.google.com/

Show 15 quoted lines

> ---
>  t/t3903-stash.sh | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 8d002a7f2..b78db74ae 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -1064,6 +1064,27 @@ test_expect_success 'push: <pathspec> not in the repository errors out' '
>  	test_path_is_file untracked
>  '
>  
> +test_expect_success 'push: -q is quiet with changes' '
> +	>foo &&
> +	git stash push -q >output 2>&1 &&

We create an untracked file here and then call 'git stash push', which will not create a new stash, as we don't use the --include-untracked option. In fact, right now this test is doing the same thing as the test below. There should be a 'git add foo' above the 'git stash push' call to test what we're claiming to test here.

Show 12 quoted lines

> +	test_must_be_empty output
> +'
> +
> +test_expect_success 'push: -q is quiet with no changes' '
> +	git stash push -q >output 2>&1 &&
> +	test_must_be_empty output
> +'
> +
> +test_expect_success 'push: -q is quiet even if there is no initial commit' '
> +	git init foo_dir &&
> +	cd foo_dir &&
> +	touch bar &&

The typical style in the test suite for creating a new file is to use '>bar', unless you care about the 'mtime' the file has. We don't seem to care about that in this test, so avoiding 'touch' would be better.

> +	test_must_fail git stash push -q >output 2>&1 &&
> +	test_must_be_empty output &&
> +	cd .. &&

The above should be in a subshell, i.e.

    (
        cd foo_dir &&
	touch bar &&
        test_must_fail git stash push -q >output 2>&1 &&
        test_must_be_empty output &&
    )

then you don't have to do the 'cd ..' in the end. With the 'cd ..' in the end, if one of the commands between the 'cd foo_dir' and 'cd ..' fails, all subsequent tests will be run inside of 'foo_dir', which puts them in a different environment than they expect. That can cause all kinds of weirdness.

If inside a subshell, the current working directory of the parent
shell is unaffected, so we don't have to worry about cd'ing back, and
subsequent tests will get the correct cwd even if things go wrong in
this test.

> +	rm -rf foo_dir

We'll want to run this cleanup to run even if the test fails. To do so, the 'test_when_finished' helper can be used. Using that, this would go at the top of the test, as 'test_when_finished rm -rf foo_dir'. Otherwise if any of the commands above fail, 'foo_dir' will not be removed, and may interfere with subsequent tests.

Show 8 quoted lines

> +'
> +
>  test_expect_success 'untracked files are left in place when -u is not given' '
>  	>file &&
>  	git add file &&
> -- 
> 2.18.0.573.g56500d98f
>