Re: [PATCH v8 09/11] convert: modernize tests

["Jakub Narębski" <jnareb@gmail.com>]

W dniu 20.09.2016 o 21:02, larsxschneider@gmail.com pisze:
> From: Lars Schneider <larsxschneider@gmail.com>
> 
> Use `test_config` to set the config, check that files are empty with
> `test_must_be_empty`, compare files with `test_cmp`, and remove spaces
> after ">" and "<".

That's good.

> 
> Please note that the "rot13" filter configured in "setup" keeps using
> `git config` instead of `test_config` because subsequent tests might
> depend on it.

This is good information to have for doing review (which could include
"post-mortem" review during bisect, so it should be in commit message
proper).

> 
> Reviewed-by: Stefan Beller <sbeller@google.com>

I have not reviewed this patch in detail, but it looks good.
A bit of nitpicking below.

> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
>  t/t0021-conversion.sh | 58 +++++++++++++++++++++++++--------------------------
>  1 file changed, 29 insertions(+), 29 deletions(-)
> 
> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
> index e799e59..dc50938 100755
> --- a/t/t0021-conversion.sh
> +++ b/t/t0021-conversion.sh
> @@ -38,8 +38,8 @@ script='s/^\$Id: \([0-9a-f]*\) \$/\1/p'
>  
>  test_expect_success check '

This patch is "while at it" already for this patch series, done
I guess for new tests to both use modern style, and be consistent
with the rest of test...

...that said, if you could modernize _naming_ of tests.  The t0021
test is quite inconsistent, and uses:

 * standard short names, like 'setup', without quotes (once),
   which is I think all right
 * cryptic short names, like 'check', without quotes (once)
 * snake_case name, like 'expanded_in_repo', without quotes (once)
 
>  test_expect_success "filter: clean empty file" '
>  test_expect_success "filter: smudge empty file" '

 * double quoted names (twice, see above)
 * proper modern names, with single quotes (the rest),
   which is as almost all the rest should be using

Best,
-- 
Jakub Narębski