Skip to content

Commit f2e2136

Browse files
gitstergitster
committed
Merge branch 'md/test-cleanup'
Various test scripts have been updated for style and also correct handling of exit status of various commands. * md/test-cleanup: tests: order arguments to git-rev-list properly t9109: don't swallow Git errors upstream of pipes tests: don't swallow Git errors upstream of pipes t/*: fix ordering of expected/observed arguments tests: standardize pipe placement Documentation: add shell guidelines t/README: reformat Do, Don't, Keep in mind lists
2 parents fb468f0 + 8d6ba49 commit f2e2136

39 files changed

+568
-399
lines changed

‎Documentation/CodingGuidelines‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,24 @@ For shell scripts specifically (not exhaustive):
118118
do this
119119
fi
120120

121+
- If a command sequence joined with && or || or | spans multiple
122+
lines, put each command on a separate line and put && and || and |
123+
operators at the end of each line, rather than the start. This
124+
means you don't need to use \ to join lines, since the above
125+
operators imply the sequence isn't finished.
126+
127+
(incorrect)
128+
grep blob verify_pack_result \
129+
| awk -f print_1.awk \
130+
| sort >actual &&
131+
...
132+
133+
(correct)
134+
grep blob verify_pack_result |
135+
awk -f print_1.awk |
136+
sort >actual &&
137+
...
138+
121139
- We prefer "test" over "[ ... ]".
122140

123141
- We do not write the noiseword "function" in front of shell

‎t/README‎

Lines changed: 47 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -401,13 +401,13 @@ This test harness library does the following things:
401401
consistently when command line arguments --verbose (or -v),
402402
--debug (or -d), and --immediate (or -i) is given.
403403

404-
Do's, don'ts & things to keep in mind
405-
-------------------------------------
404+
Do's & don'ts
405+
-------------
406406

407407
Here are a few examples of things you probably should and shouldn't do
408408
when writing tests.
409409

410-
Do:
410+
Here are the "do's:"
411411

412412
- Put all code inside test_expect_success and other assertions.
413413

@@ -452,43 +452,75 @@ Do:
452452
Windows, where the shell (MSYS bash) mangles absolute path names.
453453
For details, see the commit message of 4114156ae9.
454454

455-
Don't:
455+
- Remember that inside the <script> part, the standard output and
456+
standard error streams are discarded, and the test harness only
457+
reports "ok" or "not ok" to the end user running the tests. Under
458+
--verbose, they are shown to help debug the tests.
459+
460+
And here are the "don'ts:"
456461

457-
- exit() within a <script> part.
462+
- Don't exit() within a <script> part.
458463

459464
The harness will catch this as a programming error of the test.
460465
Use test_done instead if you need to stop the tests early (see
461466
"Skipping tests" below).
462467

463-
- use '! git cmd' when you want to make sure the git command exits
464-
with failure in a controlled way by calling "die()". Instead,
468+
- Don't use '! git cmd' when you want to make sure the git command
469+
exits with failure in a controlled way by calling "die()". Instead,
465470
use 'test_must_fail git cmd'. This will signal a failure if git
466471
dies in an unexpected way (e.g. segfault).
467472

468473
On the other hand, don't use test_must_fail for running regular
469474
platform commands; just use '! cmd'. We are not in the business
470475
of verifying that the world given to us sanely works.
471476

472-
- use perl without spelling it as "$PERL_PATH". This is to help our
473-
friends on Windows where the platform Perl often adds CR before
477+
- Don't feed the output of a git command to a pipe, as in:
478+
479+
git -C repo ls-files |
480+
xargs -n 1 basename |
481+
grep foo
482+
483+
which will discard git's exit code and may mask a crash. In the
484+
above example, all exit codes are ignored except grep's.
485+
486+
Instead, write the output of that command to a temporary
487+
file with ">" or assign it to a variable with "x=$(git ...)" rather
488+
than pipe it.
489+
490+
- Don't use command substitution in a way that discards git's exit
491+
code. When assigning to a variable, the exit code is not discarded,
492+
e.g.:
493+
494+
x=$(git cat-file -p $sha) &&
495+
...
496+
497+
is OK because a crash in "git cat-file" will cause the "&&" chain
498+
to fail, but:
499+
500+
test "refs/heads/foo" = "$(git symbolic-ref HEAD)"
501+
502+
is not OK and a crash in git could go undetected.
503+
504+
- Don't use perl without spelling it as "$PERL_PATH". This is to help
505+
our friends on Windows where the platform Perl often adds CR before
474506
the end of line, and they bundle Git with a version of Perl that
475507
does not do so, whose path is specified with $PERL_PATH. Note that we
476508
provide a "perl" function which uses $PERL_PATH under the hood, so
477509
you do not need to worry when simply running perl in the test scripts
478510
(but you do, for example, on a shebang line or in a sub script
479511
created via "write_script").
480512

481-
- use sh without spelling it as "$SHELL_PATH", when the script can
482-
be misinterpreted by broken platform shell (e.g. Solaris).
513+
- Don't use sh without spelling it as "$SHELL_PATH", when the script
514+
can be misinterpreted by broken platform shell (e.g. Solaris).
483515

484-
- chdir around in tests. It is not sufficient to chdir to
516+
- Don't chdir around in tests. It is not sufficient to chdir to
485517
somewhere and then chdir back to the original location later in
486518
the test, as any intermediate step can fail and abort the test,
487519
causing the next test to start in an unexpected directory. Do so
488520
inside a subshell if necessary.
489521

490-
- save and verify the standard error of compound commands, i.e. group
491-
commands, subshells, and shell functions (except test helper
522+
- Don't save and verify the standard error of compound commands, i.e.
523+
group commands, subshells, and shell functions (except test helper
492524
functions like 'test_must_fail') like this:
493525

494526
( cd dir && git cmd ) 2>error &&
@@ -503,7 +535,7 @@ Don't:
503535
( cd dir && git cmd 2>../error ) &&
504536
test_cmp expect error
505537

506-
- Break the TAP output
538+
- Don't break the TAP output
507539

508540
The raw output from your test may be interpreted by a TAP harness. TAP
509541
harnesses will ignore everything they don't know about, but don't step
@@ -523,13 +555,6 @@ Don't:
523555
but the best indication is to just run the tests with prove(1),
524556
it'll complain if anything is amiss.
525557

526-
Keep in mind:
527-
528-
- Inside the <script> part, the standard output and standard error
529-
streams are discarded, and the test harness only reports "ok" or
530-
"not ok" to the end user running the tests. Under --verbose, they
531-
are shown to help debugging the tests.
532-
533558

534559
Skipping tests
535560
--------------

‎t/lib-gpg.sh‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -57,9 +57,12 @@ then
5757
echo | gpgsm --homedir "${GNUPGHOME}" 2>/dev/null \
5858
--passphrase-fd 0 --pinentry-mode loopback \
5959
--import "$TEST_DIRECTORY"/lib-gpg/gpgsm_cert.p12 &&
60-
gpgsm --homedir "${GNUPGHOME}" 2>/dev/null -K \
61-
| grep fingerprint: | cut -d" " -f4 | tr -d '\n' > \
62-
${GNUPGHOME}/trustlist.txt &&
60+
61+
gpgsm --homedir "${GNUPGHOME}" 2>/dev/null -K |
62+
grep fingerprint: |
63+
cut -d" " -f4 |
64+
tr -d '\n' >"${GNUPGHOME}/trustlist.txt" &&
65+
6366
echo " S relax" >> ${GNUPGHOME}/trustlist.txt &&
6467
(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&
6568
echo hello | gpgsm --homedir "${GNUPGHOME}" >/dev/null \

‎t/t0000-basic.sh‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1097,7 +1097,7 @@ test_expect_success 'validate git diff-files output for a know cache/work tree s
10971097
:120000 120000 $(test_oid subp3s) $ZERO_OID M path3/subp3/file3sym
10981098
EOF
10991099
git diff-files >current &&
1100-
test_cmp current expected
1100+
test_cmp expected current
11011101
'
11021102

11031103
test_expect_success 'git update-index --refresh should succeed' '

‎t/t0021-conversion.sh‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -166,10 +166,10 @@ test_expect_success expanded_in_repo '
166166
rm -f expanded-keywords expanded-keywords-crlf &&
167167
168168
git checkout -- expanded-keywords &&
169-
test_cmp expanded-keywords expected-output &&
169+
test_cmp expected-output expanded-keywords &&
170170
171171
git checkout -- expanded-keywords-crlf &&
172-
test_cmp expanded-keywords-crlf expected-output-crlf
172+
test_cmp expected-output-crlf expanded-keywords-crlf
173173
'
174174

175175
# The use of %f in a filter definition is expanded to the path to

‎t/t1006-cat-file.sh‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -220,17 +220,17 @@ test_expect_success "--batch-check for a non-existent hash" '
220220
test "0000000000000000000000000000000000000042 missing
221221
0000000000000000000000000000000000000084 missing" = \
222222
"$( ( echo 0000000000000000000000000000000000000042;
223-
echo_without_newline 0000000000000000000000000000000000000084; ) \
224-
| git cat-file --batch-check)"
223+
echo_without_newline 0000000000000000000000000000000000000084; ) |
224+
git cat-file --batch-check)"
225225
'
226226

227227
test_expect_success "--batch for an existent and a non-existent hash" '
228228
test "$tag_sha1 tag $tag_size
229229
$tag_content
230230
0000000000000000000000000000000000000000 missing" = \
231231
"$( ( echo $tag_sha1;
232-
echo_without_newline 0000000000000000000000000000000000000000; ) \
233-
| git cat-file --batch)"
232+
echo_without_newline 0000000000000000000000000000000000000000; ) |
233+
git cat-file --batch)"
234234
'
235235

236236
test_expect_success "--batch-check for an empty line" '

‎t/t1300-config.sh‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1001,7 +1001,7 @@ EOF
10011001

10021002
test_expect_success 'value continued on next line' '
10031003
git config --list > result &&
1004-
test_cmp result expect
1004+
test_cmp expect result
10051005
'
10061006

10071007
cat > .git/config <<\EOF
@@ -1770,8 +1770,9 @@ test_expect_success '--show-origin stdin with file include' '
17701770
cat >expect <<-EOF &&
17711771
file:$INCLUDE_DIR/stdin.include include
17721772
EOF
1773-
echo "[include]path=\"$INCLUDE_DIR\"/stdin.include" \
1774-
| git config --show-origin --includes --file - user.stdin >output &&
1773+
echo "[include]path=\"$INCLUDE_DIR\"/stdin.include" |
1774+
git config --show-origin --includes --file - user.stdin >output &&
1775+
17751776
test_cmp expect output
17761777
'
17771778

@@ -1881,7 +1882,7 @@ test_expect_success '--replace-all does not invent newlines' '
18811882
Qkey = b
18821883
EOF
18831884
git config --replace-all abc.key b &&
1884-
test_cmp .git/config expect
1885+
test_cmp expect .git/config
18851886
'
18861887

18871888
test_done

‎t/t1303-wacky-config.sh‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,15 +14,15 @@ setup() {
1414
check() {
1515
echo "$2" >expected
1616
git config --get "$1" >actual 2>&1
17-
test_cmp actual expected
17+
test_cmp expected actual
1818
}
1919

2020
# 'check section.key regex value' verifies that the entry for
2121
# section.key *that matches 'regex'* is 'value'
2222
check_regex() {
2323
echo "$3" >expected
2424
git config --get "$1" "$2" >actual 2>&1
25-
test_cmp actual expected
25+
test_cmp expected actual
2626
}
2727

2828
test_expect_success 'modify same key' '

‎t/t2101-update-index-reupdate.sh‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ test_expect_success 'update-index --update from subdir' '
7373
100644 $(git hash-object dir1/file3) 0 dir1/file3
7474
100644 $file2 0 file2
7575
EOF
76-
test_cmp current expected
76+
test_cmp expected current
7777
'
7878

7979
test_expect_success 'update-index --update with pathspec' '

‎t/t3200-branch.sh‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1221,7 +1221,7 @@ test_expect_success 'use --edit-description' '
12211221
EOF
12221222
EDITOR=./editor git branch --edit-description &&
12231223
echo "New contents" >expect &&
1224-
test_cmp EDITOR_OUTPUT expect
1224+
test_cmp expect EDITOR_OUTPUT
12251225
'
12261226

12271227
test_expect_success 'detect typo in branch name when using --edit-description' '

0 commit comments

Comments
 (0)