Re: [PATCH 3/3] ls-tree: add unit tests for arguments

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, Jul 2, 2018 at 11:58 PM Joshua Nelson <jyn514@gmail.com> wrote:
> Signed-off-by: Joshua Nelson <jyn514@gmail.com>
> ---
> diff --git t/t3104-ls-tree-optional-args.sh t/t3104-ls-tree-optional-args.sh
> @@ -0,0 +1,43 @@
> +test_expect_success 'initial setup' '
> +echo hi > test && cp test test2 && git add test test2 && git commit -m initial'

As this is a new test script, please use modern formatting style:
indent test body with tabs, closing single-quote goes on its own line,
break lines at &&, and no whitespace between > and the filename. Also,
it's customary to simply to call the test "setup".

test_expect_success 'setup' '
    echo hi >test &&
    cp test test2 &&
    git add test test2 &&
    git commit -m initial
'

> +# cat appends newlines after every file

Why is this talking about a "cat"? Doesn't seem relevant.

> +test_expect_success 'succeed when given no args' 'git ls-tree'

This test seems too minimal. As the intention of this patch series is
to make git-ls-tree default to HEAD when no treeish is given, I would
expect the test with no arguments to verify that it did indeed list
the tree associated with HEAD. As implemented, this test tells us
nothing other than that it didn't error out or crash.

> +test_expect_success 'succeed when given only --' 'git ls-tree'

Um, what's this supposed to be testing? Presently, it seems to
duplicate the previous test. I'm guessing it should be running "git
ls-tree --" instead.

> +test_expect_success 'add second commit' '
> +echo hi > test3 && git add test3 && git commit -m "commit 2"'
> +
> +test_expect_success 'succeed when given revision' '
> +git ls-tree HEAD~1'

Given how patch 1/3 makes some fundamental changes to how git-ls-tree
processes its arguments, I would again expect this test to verify that
it indeed lists the correct tree. As the test is currently
implemented, we have no way of knowing what tree (if any) it listed.

> +test_expect_success 'succeed when given revision and --' '
> +git ls-tree HEAD~1 --'
> +
> +test_expect_success 'succeed when given -- and file' '
> +git ls-tree -- test3'

As above, given the fundamental changes to argument processing, I'd
expect this to verify that the output of this command is indeed what
is expected.

> +test_expect_success 'do nothing when given bad files' '
> +git ls-tree -- bad_files'

I wonder about this. Is it just an accident of implementation that
git-ls-tree doesn't error out in this case, or is it intended behavior
that it should return 0 even when the file is not in the tree? If the
0 exit code is just an accident of implementation, then we shouldn't
be enforcing this by a test (instead, someone perhaps ought to fix
git-ls-tree).

> +test_expect_success 'succeed when given file from past revision' '
> +git ls-tree HEAD~1 test'

Same comment as above about verifying gave expected output.

> +test_expect_success 'succeed when given only file' 'git ls-tree test'
> +
> +test_expect_success 'raise error when given bad args' '
> +test_must_fail  git ls-tree HEAD HEAD --'
> +
> +test_expect_success 'raise error when given bad revision' '
> +test_must_fail git ls-tree bad_revision --'