[RFC,v4,3/3] t0014: Introduce alias testing suite
diff mbox series

Message ID 20180907224430.23859-3-timschumi@gmx.de
State New
Headers show
Series
  • [RFC,v4,1/3] Add support for nested aliases
Related show

Commit Message

Tim Schumacher Sept. 7, 2018, 10:44 p.m. UTC
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>
---

Those are the tests that I've come up with. It consists of tests
for nested aliases and looping aliases, both with internal calls
and external calls.

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?

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.

 t/t0014-alias.sh | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100755 t/t0014-alias.sh

Comments

Eric Sunshine Sept. 7, 2018, 11:38 p.m. UTC | #1
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.)

> +'
Tim Schumacher Sept. 14, 2018, 11:12 p.m. UTC | #2
On 08.09.18 01:38, Eric Sunshine wrote:
> 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.

That will probably be the easiest thing to do. I commented it out for
now, added a short information about that to the code itself and a longer
explanation to the commit message.

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

Backslash-escaping didn't work, that resulted in some parsing error.
I'm using i18ngrep now to search for the part of a message, which
eliminates the need for quotes completely.

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

Whoops, I didn't know when that went missing. I added it into a new version
of this patch.

Also, I decided to keep `git status`, because it seemed to be the only
command which doesn't need any files to produce some checkable output.
Checking the "On branch" message should be enough to confirm that the
command works as intended.

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

I incorporated both of these suggestions.

> 
>> +'
> 

This is the first multi-patch series that I submitted, so I'm unsure if I
should send the updated patch only or if I should send the complete series
again as v5. Any pointers to what the correct procedure for this case is would
be appreciated.

Thanks for looking at this.

Tim
Eric Sunshine Sept. 16, 2018, 7:21 a.m. UTC | #3
On Fri, Sep 14, 2018 at 7:12 PM Tim Schumacher <timschumi@gmx.de> wrote:
> This is the first multi-patch series that I submitted, so I'm unsure if I
> should send the updated patch only or if I should send the complete series
> again as v5. Any pointers to what the correct procedure for this case is would
> be appreciated.

Re-send the entire series as v5. That makes it easier on reviewers
(who don't need to go searching through the mailing list archive to
get a full picture) and reduces Junio's workload since it's usually
easier for him to re-queue a series wholesale than having to
slice-and-dice some replacement patches into what was already queued.

Patch
diff mbox series

diff --git a/t/t0014-alias.sh b/t/t0014-alias.sh
new file mode 100755
index 000000000..6c1e34694
--- /dev/null
+++ 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
+'
+
+test_expect_success 'nested aliases - internal execution' '
+	git config alias.nested-internal-1 nested-internal-2 &&
+	git config alias.nested-internal-2 status
+'
+
+test_expect_success 'nested aliases - mixed execution' '
+	git config alias.nested-external-1 "!git nested-external-2" &&
+	git config alias.nested-external-2 status
+'
+
+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 &&
+	rm output
+'
+
+test_expect_failure 'looping aliases - mixed execution' '
+	git config alias.loop-mixed-1 loop-mixed-2 &&
+	git config alias.loop-mixed-2 "!git loop-mixed-1" &&
+	test_must_fail git loop-mixed-1 2>output &&
+	grep -q "fatal: alias loop detected: expansion of '"'"'loop-mixed-1'"'"' does not terminate" output &&
+	rm output
+'
+
+test_done