Ramkumar Ramachandra wrote:
> Put the opening quote starting each test on the same line as the
> test_expect_* invocation. While at it:
[...]
> - Use <<\-EOF in preference to <<EOF to save readers the trouble of
> looking for variable interpolations.
I think you mean <<-\EOF. :)
[...]
> - Chain commands with &&. Breaks in a test assertion's && chain can
> potentially hide failures from earlier commands in the chain.
>
> - Use test_expect_code() in preference to checking the exit status of
> various statements by hand.
I guess these two are the motivation?
> Inspired-by: Jonathan Nieder <jrnieder@xxxxxxxxx>
Oh, dear.
> Signed-off-by: Ramkumar Ramachandra <artagnon@xxxxxxxxx>
[...]
> --- a/t/t3040-subprojects-basic.sh
> +++ b/t/t3040-subprojects-basic.sh
> @@ -3,81 +3,81 @@
> test_description='Basic subproject functionality'
> . ./test-lib.sh
>
> -test_expect_success 'Super project creation' \
> - ': >Makefile &&
> - git add Makefile &&
> - git commit -m "Superproject created"'
> -
> -
> -cat >expected <<EOF
[...]
It would be easier to read if each preimage test assertion were next to
the corresponding postimage test assertion. Does "git diff --patience"
do better?
> -test_expect_success 'Super project creation' \
> - ': >Makefile &&
> - git add Makefile &&
> - git commit -m "Superproject created"'
> -
> -
> +test_expect_success 'setup: create superproject' '
> + : >Makefile &&
> + git add Makefile &&
> + git commit -m "Superproject created"
> +'
> +
Ok, makes sense.
> -cat >expected <<EOF
> -:000000 160000 00000... A sub1
> -:000000 160000 00000... A sub2
> -EOF
> -test_expect_success 'create subprojects' \
> - 'mkdir sub1 &&
> - ( cd sub1 && git init && : >Makefile && git add * &&
> - git commit -q -m "subproject 1" ) &&
> +test_expect_success 'setup: create subprojects' '
> + mkdir sub1 &&
> + ( cd sub1 && git init && : >Makefile && git add * &&
> + git commit -q -m "subproject 1" ) &&
If cleaning up the style anyway, I would write this as
mkdir sub1 &&
(
cd sub1 &&
git init &&
>Makefile &&
git add Makefile &&
git commit -m "subproject 1"
)
Or
mkdir sub1 &&
(
cd sub1 &&
git init &&
test_commit subproject-1 Makefile
)
But leaving it alone like you did is probably better. ;-)
[...]
> - git diff-tree --abbrev=5 HEAD^ HEAD |cut -d" " -f-3,5- >current &&
> - test_cmp expected current'
> -
> -git branch save HEAD
> -
> + git diff-tree --abbrev=5 HEAD^ HEAD |cut -d" " -f-3,5- >current &&
> + git branch save HEAD &&
> + cat >expected <<-\EOF &&
> + :000000 160000 00000... A sub1
> + :000000 160000 00000... A sub2
> + EOF
> + test_cmp expected current
> +'
> +
At first I wondered if "git branch save HEAD" is logically part of the
same test. After all, it's just meant as a baseline for use by later
tests.
After thinking about it for a few seconds, though, that's exactly what
this test is about. Maybe it would have been clearer if the two setup
tests were combined into one (but please don't take this advice too
seriously; I'm just musing).
> -test_expect_success 'check if fsck ignores the subprojects' \
> - 'git fsck --full'
> +test_expect_success 'check if fsck ignores the subprojects' '
> + git fsck --full
> +'
Does this test imply that one of the subprojects is broken somehow?
> -
> -test_expect_success 'check if commit in a subproject detected' \
> - '( cd sub1 &&
> - echo "all:" >>Makefile &&
> - echo " true" >>Makefile &&
> - git commit -q -a -m "make all" ) && {
> - git diff-files --exit-code
> - test $? = 1
> - }'
> +
> +test_expect_success 'check if commit in a subproject detected' '
> + ( cd sub1 &&
> + echo "all:" >>Makefile &&
> + echo " true" >>Makefile &&
> + git commit -q -a -m "make all" ) &&
> + test_expect_code 1 git diff-files --exit-code
> +'
Nice. Style again: I'd be tempted to reformat as
(
cd sub1 &&
echo "all:" >>Makefile &&
...
) &&
test_expect_code 1 git diff-files --exit-code
to make the subshell scope a little clearer, but exercising restraint
like you did may be better.
[...]
>
> # the index must contain the object name the HEAD of the
> # subproject sub1 was at the point "save"
> -test_expect_success 'checkout in superproject' \
> - 'git checkout save &&
> - git diff-index --exit-code --raw --cached save -- sub1'
> +test_expect_success 'checkout in superproject' '
> + git checkout save &&
> + git diff-index --exit-code --raw --cached save -- sub1
> +'
Thanks much. The result really is a little easier on the eyes, and
the changes look safe.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html