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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 3, 2018, 07:33 UTC
Message-ID: <CAPig+cSZbGSsUvXmM1+q7ZhHmRg75tuM+=KyuzG5NFuzOdbYVA@mail.gmail.com>
In-Reply-To: <20180703035802.24060-3-jyn514@gmail.com>

On Mon, Jul 2, 2018 at 11:58 PM Joshua Nelson <jyn514@gmail.com> wrote:

Show 6 quoted lines

> 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.

Show 5 quoted lines

> +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.

Show 5 quoted lines

> +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.

Show 7 quoted lines

> +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 --'