git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 1/4] tests: run internal chain-linter under "make test"

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Mar 29, 2023, 10:20 UTC
Message-ID
<230329.86wn2zvo9u.gmgdl@evledraar.gmail.com>
In-Reply-To
<20230328202207.GA1241631@coredump.intra.peff.net>
On Tue, Mar 28 2023, Jeff King wrote:
Show 70 quoted lines
> Since 69b9924b875 (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 <peff@peff.net>
> ---
>  t/Makefile    | 2 +-
>  t/test-lib.sh | 3 ++-
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/t/Makefile b/t/Makefile
> index 88fa5049573..10881affdd0 100644
> --- a/t/Makefile
> +++ b/t/Makefile
> @@ -45,7 +45,7 @@ CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
>  # `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`)
>  # checks all tests in all scripts via a single invocation, so tell individual
>  # scripts not to "chainlint" themselves
> -CHAINLINTSUPPRESS = GIT_TEST_CHAIN_LINT=0 && export GIT_TEST_CHAIN_LINT &&
> +CHAINLINTSUPPRESS = GIT_TEST_EXT_CHAIN_LINT=0 && export GIT_TEST_EXT_CHAIN_LINT &&
>  
>  all: $(DEFAULT_TEST_TARGET)
>  
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 62136caee5a..09789566374 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1593,7 +1593,8 @@ then
>  	BAIL_OUT_ENV_NEEDS_SANITIZE_LEAK "GIT_TEST_SANITIZE_LEAK_LOG=true"
>  fi
>  
> -if test "${GIT_TEST_CHAIN_LINT:-1}" != 0
> +if test "${GIT_TEST_CHAIN_LINT:-1}" != 0 &&
> +   test "${GIT_TEST_EXT_CHAIN_LINT:-1}" != 0
>  then
>  	"$PERL_PATH" "$TEST_DIRECTORY/chainlint.pl" "$0" ||
>  		BUG "lint error (see '?!...!? annotations above)"

So, we used to use the "sed" script to run against the *.sh, and do the "eval 117" check, but since chainlint.pl we've checked all the tests in one go when invoked from the Makefile, and turned off the "eval 117".

And, as this change points out, there's cases where chainlint.pl won't catch everything, so we'd like to keep the "eval" bits by default, but not the invoking of chainlint.pl by the script itself.

All of which makes sense.

But it seems to me that the findings in this change make the docs outdated, and we should update them, but maybe I'm missing something.

Now the comment in the Makefile still says (as seen in the context):
	# `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`)
	# checks all tests in all scripts via a single invocation, so tell individual
	# scripts not to "chainlint" themselves

But as the commit message notes that's not accurate anymore. We're *not* telling them to turn off chainlint in its entirety, we're telling them to only suppress the chainlint.pl part.

So if you invoke the Makefile with GIT_TEST_CHAIN_LINT=0 we'll still turn all of it off, but an invokation of chainlint.pl for the scripts as a batch is no longer thought to make the "eval 117" trick redundant.

I haven't looked too deeply, but it seems that we should at least adjust that comment in the Makefile, or perhaps we should rename this "eval 117" thing to be "internal lint" or something, and not "chain lint", to avoid conflating it with chainlint.pl itself.

So maybe something like this, i.e. we'd just "rebrand" this to begin with, and avoid editing the t/Makefile at all (untested)?

 t/README      | 12 ++++++++++++
 t/test-lib.sh |  6 +++++-
 2 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/t/README b/t/README
index 29576c37488..83d58f2e680 100644
--- a/t/README
+++ b/t/README
@@ -196,6 +196,18 @@ appropriately before running "make". Short options can be bundled, i.e.
 	this feature by setting the GIT_TEST_CHAIN_LINT environment
 	variable to "1" or "0", respectively.
 
+--internal-lint::
+--no-internal-lint::
+	If --internal-lint is enabled, the test harness will try to
+	detect missing "&&"-chains in edge cases that "--chain-lint"
+	wouldn't notice, unlike "--chain-lint" this check is run from
+	within "test_expect_*" itself, and thus requires the tests to
+	run.
+
+	You may also enable or disable this feature by setting the
+	GIT_TEST_INTERNAL_LINT environment variable to "1" or "0",
+	respectively.
+
 --stress::
 	Run the test script repeatedly in multiple parallel jobs until
 	one of them fails.  Useful for reproducing rare failures in
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 62136caee5a..792c68aa56a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -205,6 +205,10 @@ parse_option () {
 		GIT_TEST_CHAIN_LINT=1 ;;
 	--no-chain-lint)
 		GIT_TEST_CHAIN_LINT=0 ;;
+	--internal-lint)
+		GIT_TEST_INTERNAL_LINT=1 ;;
+	--no-internal-lint)
+		GIT_TEST_INTERNAL_LINT=0 ;;
 	-x)
 		trace=t ;;
 	-V|--verbose-log)
@@ -1090,7 +1094,7 @@ test_run_ () {
 	test_cleanup=:
 	expecting_failure=$2
 
-	if test "${GIT_TEST_CHAIN_LINT:-1}" != 0; then
+	if test "${GIT_TEST_INTERNAL_LINT:-1}" != 0; then
 		# turn off tracing for this test-eval, as it simply creates
 		# confusing noise in the "-x" output
 		trace_tmp=$trace
Previous: Jeff KingNext: Junio C Hamano
Message 3 of 32 in “some chainlint fixes and performance improvements”
  1. 0/4 some chainlint fixes and performance improvementsJeff King, Mar 28, 2023
  2. 1/4 tests: run internal chain-linter under "make test"Jeff King, Mar 28, 2023
  3. Ævar Arnfjörð BjarmasonMar 29, 2023
  4. Junio C HamanoMar 29, 2023
  5. Jeff KingMar 29, 2023
  6. Junio C HamanoMar 30, 2023
  7. 2/4 tests: replace chainlint subshell with a functionJeff King, Mar 28, 2023
  8. Junio C HamanoMar 28, 2023
  9. 3/4 tests: drop here-doc check from internal chain-linterJeff King, Mar 28, 2023
  10. Junio C HamanoMar 28, 2023
  11. Jeff KingMar 29, 2023
  12. Jeff KingMar 29, 2023
  13. Eric SunshineMar 29, 2023
  14. Eric SunshineMar 29, 2023
  15. Eric SunshineMar 29, 2023
  16. Jeff KingMar 29, 2023
  17. Eric SunshineMar 29, 2023
  18. Eric SunshineMar 29, 2023
  19. Jeff KingMar 29, 2023
  20. 4/4 tests: skip test_eval_ in internal chain-lintJeff King, Mar 28, 2023
  21. Jeff KingMar 28, 2023
  22. Jeff KingMar 30, 2023
  23. Junio C HamanoMar 30, 2023
  24. 0/5 some chainlint fixes and performance improvementsJeff King, Mar 30, 2023
  25. 1/5 tests: run internal chain-linter under "make test"Jeff King, Mar 30, 2023
  26. 2/5 tests: replace chainlint subshell with a functionJeff King, Mar 30, 2023
  27. 3/5 tests: diagnose unclosed here-doc in chainlint.plJeff King, Mar 30, 2023
  28. Eric SunshineMar 30, 2023
  29. 4/5 tests: drop here-doc check from internal chain-linterJeff King, Mar 30, 2023
  30. 5/5 tests: skip test_eval_ in internal chain-lintJeff King, Mar 30, 2023
  31. Junio C HamanoMar 30, 2023
  32. Jeff KingMar 30, 2023

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.