Re: [RFC PATCH v4 3/3] t0014: Introduce alias testing suite

> On Fri, Sep 7, 2018 at 6:44 PM Tim Schumacher <timschumi@gmx.de> wrote:
>> Introduce a testing suite that is dedicated to aliases.
>> For now, check only if nested aliases work and if looping
>> aliases are detected successfully.
>>
>> The looping aliases check for mixed execution is there but
>> expected to fail because there is no check in place yet.
>>
>> Signed-off-by: Tim Schumacher <timschumi@gmx.de>
>> ---
>> Unfortunately I don't have a fix for the last one yet, so I
>> marked it as expect_failure. The problem is that the test suite
>> is waiting a full minute until it aborts the running command
>> (which I guess should not take that long, as it blocks the whole
>> test suite for that span of time).
>>
>> Should I try to decrease the timeout or should I remove that
>> test completely until I manage to get external calls fixed?
> 
> Perhaps just comment out that test for now and add a comment above it
> explaining why it's commented out.

> 
>> As a last thing, is there any better way to use single quotes
>> than to write '"'"'? It isn't that bad, but it is hard to read,
>> especially for bash newcomers.
> 
> You should backslash-escape the quotes ("foo \'bar\' baz"), however,
> in this case, it would make sense to use regex's with 'grep' to check
> that you got the expected error message rather than reproducing the
> message literally here in the script.

> 
> More below.
> 
>> diff --git a/t/t0014-alias.sh b/t/t0014-alias.sh
>> @@ -0,0 +1,38 @@
>> +#!/bin/sh
>> +
>> +test_description='git command aliasing'
>> +
>> +. ./test-lib.sh
>> +
>> +test_expect_success 'setup environment' '
>> +       git init
>> +'
> 
> "git init" is invoked automatically by the test framework, so no need
> for this test. You can drop it.
> 
>> +test_expect_success 'nested aliases - internal execution' '
>> +       git config alias.nested-internal-1 nested-internal-2 &&
>> +       git config alias.nested-internal-2 status
>> +'
> 
> This isn't actually testing anything, is it? It's setting up the
> aliases but never actually invoking them. I would have expected the
> next line to actually run a command ("git nested-internal-1") and the
> line after that to check that you got the expected output (whatever
> "git status" would emit). Output from "git status" isn't necessarily
> the easiest to test, though, so perhaps pick a different Git command
> for testing (something for which the result can be very easily checked
> -- maybe "git rm" or such).

> 
>> +test_expect_success 'nested aliases - mixed execution' '
>> +       git config alias.nested-external-1 "!git nested-external-2" &&
>> +       git config alias.nested-external-2 status
>> +'
> 
> Same observation.
> 
>> +test_expect_success 'looping aliases - internal execution' '
>> +       git config alias.loop-internal-1 loop-internal-2 &&
>> +       git config alias.loop-internal-2 loop-internal-3 &&
>> +       git config alias.loop-internal-3 loop-internal-2 &&
>> +       test_must_fail git loop-internal-1 2>output &&
>> +       grep -q "fatal: alias loop detected: expansion of '"'"'loop-internal-1'"'"' does not terminate" output &&
> 
> Don't bother using -q with 'grep'. Output is hidden already by the
> test framework in normal mode, and not hidden when running in verbose
> mode. And, the output of 'grep' might be helpful when debugging the
> test if something goes wrong.
> 
> As noted above, you can use regex to match the expected error rather
> than exactly duplicating the text of the message.
> 
> Finally, use 'test_i18ngrep' instead of 'grep' in order to play nice
> with localization.
> 
>> +       rm output
> 
> Tests don't normally bother cleaning up their output files like this
> since such output can be helpful when debugging the test if something
> goes wrong. (You'd want to use test_when_finished to cleanup anyhow,
> but you don't need it in this case.)