Web lists-archives.com

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




Hi Joshua,

On Mon, Jul 2, 2018 at 8:58 PM, Joshua Nelson <jyn514@xxxxxxxxx> wrote:

The commit message could use some clarification; it currently makes
the reader think you're testing all arguments of ls-tree, when you're
only testing a few new ones.  Alternatively, you could squash this
with patch 1.

> Signed-off-by: Joshua Nelson <jyn514@xxxxxxxxx>
> ---
>  t/t3104-ls-tree-optional-args.sh | 43 ++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
>  create mode 100644 t/t3104-ls-tree-optional-args.sh

Please note that test files should be executable, not regular files.

>
> diff --git t/t3104-ls-tree-optional-args.sh t/t3104-ls-tree-optional-args.sh
> new file mode 100644
> index 000000000..5917563a7
> --- /dev/null
> +++ t/t3104-ls-tree-optional-args.sh
> @@ -0,0 +1,43 @@
> +#!/bin/sh
> +
> +test_description='ls-tree test (optional args)
> +
> +This test tries to run git-ls-tree with various combinations of options.'

This description seems like it's true for all the t310*.sh tests.  How
does t3104 differ from t310[0-3]*.sh?

> +
> +. ./test-lib.sh
> +
> +test_expect_success 'initial setup' '
> +echo hi > test && cp test test2 && git add test test2 && git commit -m initial'
> +
> +# cat appends newlines after every file

What is this comment in reference to?

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

I was a little surprised that these tests only check that the command
runs to completion with a 0 exit status, and do not do any
verification of the output.  I wasn't necessarily expected full output
verification, but having a few different commits and searching for
something that'd only be shown in the relevant commit would be nice.

> +
> +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'
> +
> +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'
> +
> +test_expect_success 'do nothing when given bad files' '
> +git ls-tree -- bad_files'

...and to reiterate the point above about verifying the output is
correct, how is 'doing nothing' here distinguishable from showing all
the files in the current commit if you're not checking any part of the
output?

> +
> +test_expect_success 'succeed when given file from past revision' '
> +git ls-tree HEAD~1 test'
> +
> +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 --'
> +
> +test_done
> --
> 2.18.GIT

Lots of little things to fix up, but you're off to a great start.  I'm
excited to be able to have ls-tree work without me having to specify
HEAD all the time.  :-)