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

From: Jakub Narębski <jnareb@gmail.com>
Date: Sep 25, 2016, 14:43 UTC
Message-ID: <1e2aac1a-b25d-d747-7bf6-6f842b3e438d@gmail.com>
In-Reply-To: <20160920190247.82189-10-larsxschneider@gmail.com>

W dniu 20.09.2016 o 21:02, larsxschneider@gmail.com pisze:

Show 5 quoted lines

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

Show 12 quoted lines

> 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