Skip to content

Commit 7b6555a

Browse files
peffgitster
authored andcommitted
tests: run internal chain-linter under "make test"
Since 69b9924 (t/Makefile: teach `make test` and `make prove` to run chainlint.pl, 2022-09-01), we run a single chainlint.pl process for all scripts, and then instruct each individual script to run with the equivalent of --no-chain-lint, which tells them not to redundantly run the chainlint script themselves. However, this also disables the internal linter run within the shell by eval-ing "(exit 117) && $1" and confirming we get code 117. In theory the external linter produces a superset of complaints, and we don't need the internal one anymore. However, we know there is at least one case where they differ. A test like: test_expect_success 'should fail linter' ' false && sleep 2 & pid=$! && kill $pid ' is buggy (it ignores the failure from "false", because it is backgrounded along with the sleep). The internal linter catches this, but the external one doesn't (and teaching it to do so is complicated[1]). So not only does "make test" miss this problem, but it's doubly confusing because running the script standalone does complain. Let's teach the suppression in the Makefile to only turn off the external linter (which we know is redundant, as it was already run) and leave the internal one intact. I've used a new environment variable to do this here, and intentionally did not add a "--no-ext-chain-lint" option. This is an internal optimization used by the Makefile, and not something that ordinary users would need to tweak. [1] For discussion of chainlint.pl and this case, see: https://lore.kernel.org/git/CAPig+cQtLFX4PgXyyK_AAkCvg4Aw2RAC5MmLbib-aHHgTBcDuw@mail.gmail.com/ Signed-off-by: Jeff King <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
1 parent 73876f4 commit 7b6555a

File tree

2 files changed

+4
-3
lines changed

2 files changed

+4
-3
lines changed

t/Makefile

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,8 @@ CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
4444

4545
# `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`)
4646
# checks all tests in all scripts via a single invocation, so tell individual
47-
# scripts not to "chainlint" themselves
48-
CHAINLINTSUPPRESS = GIT_TEST_CHAIN_LINT=0 && export GIT_TEST_CHAIN_LINT &&
47+
# scripts not to run the external "chainlint.pl" script themselves
48+
CHAINLINTSUPPRESS = GIT_TEST_EXT_CHAIN_LINT=0 && export GIT_TEST_EXT_CHAIN_LINT &&
4949

5050
all: $(DEFAULT_TEST_TARGET)
5151

t/test-lib.sh

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1593,7 +1593,8 @@ then
15931593
BAIL_OUT_ENV_NEEDS_SANITIZE_LEAK "GIT_TEST_SANITIZE_LEAK_LOG=true"
15941594
fi
15951595

1596-
if test "${GIT_TEST_CHAIN_LINT:-1}" != 0
1596+
if test "${GIT_TEST_CHAIN_LINT:-1}" != 0 &&
1597+
test "${GIT_TEST_EXT_CHAIN_LINT:-1}" != 0
15971598
then
15981599
"$PERL_PATH" "$TEST_DIRECTORY/chainlint.pl" "$0" ||
15991600
BUG "lint error (see '?!...!? annotations above)"

0 commit comments

Comments
 (0)