From 7b6555ab8d166545499e1f504d7b60cbd7cd8a0f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 30 Mar 2023 15:27:38 -0400 Subject: tests: run internal chain-linter under "make test" 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 Signed-off-by: Junio C Hamano --- t/Makefile | 4 ++-- t/test-lib.sh | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/t/Makefile b/t/Makefile index 2c2b252240..fd7f61cea8 100644 --- a/t/Makefile +++ b/t/Makefile @@ -44,8 +44,8 @@ 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 && +# scripts not to run the external "chainlint.pl" script themselves +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 62136caee5..0978956637 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)" -- cgit v1.2.3 From 1686de55facd0225739290e6afb51e3600351883 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 30 Mar 2023 15:27:49 -0400 Subject: tests: replace chainlint subshell with a function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit To test that we don't break the &&-chain, test-lib.sh does something like: (exit 117) && $test_commands and checks that the result is exit code 117. We don't care what that initial command is, as long as it exits with a unique code. Using "exit" works and is simple, but is a bit expensive since it requires a subshell (to avoid exiting the whole script!). This isn't usually very noticeable, but it can add up for scripts which have a large number of tests. Using "return" naively won't work here, because we'd return from the function eval-ing the snippet (and it wouldn't find &&-chain breakages). But if we further push that into its own function, it does exactly what we want, without extra subshell overhead. According to hyperfine, this produces a measurable improvement when running t3070 (which has 1800 tests, all of them quite short): 'HEAD' ran 1.09 ± 0.01 times faster than 'HEAD~1' Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/test-lib.sh | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/t/test-lib.sh b/t/test-lib.sh index 0978956637..cfcbd899c5 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -1086,6 +1086,10 @@ test_eval_ () { return $test_eval_ret_ } +fail_117 () { + return 117 +} + test_run_ () { test_cleanup=: expecting_failure=$2 @@ -1097,7 +1101,7 @@ test_run_ () { trace= # 117 is magic because it is unlikely to match the exit # code of other programs - if test "OK-117" != "$(test_eval_ "(exit 117) && $1${LF}${LF}echo OK-\$?" 3>&1)" + if test "OK-117" != "$(test_eval_ "fail_117 && $1${LF}${LF}echo OK-\$?" 3>&1)" then BUG "broken &&-chain or run-away HERE-DOC: $1" fi -- cgit v1.2.3 From 2b61c8dc8843319d09f1485fbcb3b1dc4aecb36d Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Thu, 30 Mar 2023 15:30:31 -0400 Subject: tests: diagnose unclosed here-doc in chainlint.pl An unclosed here-doc in a test is a problem, because it silently gobbles up any remaining commands. Since 99a64e4b73c (tests: lint for run-away here-doc, 2017-03-22) we detect this by piggy-backing on the internal chainlint checker in test-lib.sh. However, it would be nice to detect it in chainlint.pl, for a few reasons: - the output from chainlint.pl is much nicer; it can show the exact spot of the error, rather than a vague "somewhere in this test you broke the &&-chain or had a bad here-doc" message. - the implementation in test-lib.sh runs for each test snippet. And since it requires a subshell, the extra cost is small but not zero. If chainlint.pl can reliably find the problem, we can optimize the test-lib.sh code. The chainlint.pl code never intended to find here-doc problems. But since it has to parse them anyway (to avoid reporting problems inside here-docs), most of what we need is already there. We can detect the problem when we fail to find the missing end-tag in swallow_heredocs(). The extra change in scan_heredoc_tag() stores the location of the start of the here-doc, which lets us mark it as the source of the error in the output (see the new tests for examples). [jk: added commit message and tests] Signed-off-by: Eric Sunshine Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/chainlint.pl | 15 ++++++++++++--- t/chainlint/unclosed-here-doc-indent.expect | 4 ++++ t/chainlint/unclosed-here-doc-indent.test | 4 ++++ t/chainlint/unclosed-here-doc.expect | 7 +++++++ t/chainlint/unclosed-here-doc.test | 7 +++++++ 5 files changed, 34 insertions(+), 3 deletions(-) create mode 100644 t/chainlint/unclosed-here-doc-indent.expect create mode 100644 t/chainlint/unclosed-here-doc-indent.test create mode 100644 t/chainlint/unclosed-here-doc.expect create mode 100644 t/chainlint/unclosed-here-doc.test diff --git a/t/chainlint.pl b/t/chainlint.pl index e966412999..556ee91a15 100755 --- a/t/chainlint.pl +++ b/t/chainlint.pl @@ -80,7 +80,8 @@ sub scan_heredoc_tag { return "<<$indented" unless $token; my $tag = $token->[0]; $tag =~ s/['"\\]//g; - push(@{$self->{heretags}}, $indented ? "\t$tag" : "$tag"); + $$token[0] = $indented ? "\t$tag" : "$tag"; + push(@{$self->{heretags}}, $token); return "<<$indented$tag"; } @@ -169,10 +170,18 @@ sub swallow_heredocs { my $tags = $self->{heretags}; while (my $tag = shift @$tags) { my $start = pos($$b); - my $indent = $tag =~ s/^\t// ? '\\s*' : ''; - $$b =~ /(?:\G|\n)$indent\Q$tag\E(?:\n|\z)/gc; + my $indent = $$tag[0] =~ s/^\t// ? '\\s*' : ''; + $$b =~ /(?:\G|\n)$indent\Q$$tag[0]\E(?:\n|\z)/gc; + if (pos($$b) > $start) { + my $body = substr($$b, $start, pos($$b) - $start); + $self->{lineno} += () = $body =~ /\n/sg; + next; + } + push(@{$self->{parser}->{problems}}, ['UNCLOSED-HEREDOC', $tag]); + $$b =~ /(?:\G|\n).*\z/gc; # consume rest of input my $body = substr($$b, $start, pos($$b) - $start); $self->{lineno} += () = $body =~ /\n/sg; + last; } } diff --git a/t/chainlint/unclosed-here-doc-indent.expect b/t/chainlint/unclosed-here-doc-indent.expect new file mode 100644 index 0000000000..7c30a1a024 --- /dev/null +++ b/t/chainlint/unclosed-here-doc-indent.expect @@ -0,0 +1,4 @@ +command_which_is_run && +cat >expect <<-\EOF ?!UNCLOSED-HEREDOC?! && +we forget to end the here-doc +command_which_is_gobbled diff --git a/t/chainlint/unclosed-here-doc-indent.test b/t/chainlint/unclosed-here-doc-indent.test new file mode 100644 index 0000000000..5c841a9dfd --- /dev/null +++ b/t/chainlint/unclosed-here-doc-indent.test @@ -0,0 +1,4 @@ +command_which_is_run && +cat >expect <<-\EOF && +we forget to end the here-doc +command_which_is_gobbled diff --git a/t/chainlint/unclosed-here-doc.expect b/t/chainlint/unclosed-here-doc.expect new file mode 100644 index 0000000000..d65e50f78d --- /dev/null +++ b/t/chainlint/unclosed-here-doc.expect @@ -0,0 +1,7 @@ +command_which_is_run && +cat >expect <<\EOF ?!UNCLOSED-HEREDOC?! && + we try to end the here-doc below, + but the indentation throws us off + since the operator is not "<<-". + EOF +command_which_is_gobbled diff --git a/t/chainlint/unclosed-here-doc.test b/t/chainlint/unclosed-here-doc.test new file mode 100644 index 0000000000..69d3786c34 --- /dev/null +++ b/t/chainlint/unclosed-here-doc.test @@ -0,0 +1,7 @@ +command_which_is_run && +cat >expect <<\EOF && + we try to end the here-doc below, + but the indentation throws us off + since the operator is not "<<-". + EOF +command_which_is_gobbled -- cgit v1.2.3 From 750b2604118b4b7d9983f77adeb36d839107861f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 30 Mar 2023 15:30:47 -0400 Subject: tests: drop here-doc check from internal chain-linter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit 99a64e4b73c (tests: lint for run-away here-doc, 2017-03-22) tweaked the chain-lint test to catch unclosed here-docs. It works by adding an extra "echo" command after the test snippet, and checking that it is run (if it gets swallowed by a here-doc, naturally it is not run). The downside here is that we introduced an extra $() substitution, which happens in a subshell. This has a measurable performance impact when run for many tests. The tradeoff in safety was undoubtedly worth it when 99a64e4b73c was written. But since the external chainlint.pl learned to find these recently, we can just rely on it. By switching back to a simpler chain-lint, hyperfine reports a measurable speedup on t3070 (which has 1800 tests): 'HEAD' ran 1.12 ± 0.01 times faster than 'HEAD~1' Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/test-lib.sh | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/t/test-lib.sh b/t/test-lib.sh index cfcbd899c5..0048ec7b6f 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -1101,9 +1101,10 @@ test_run_ () { trace= # 117 is magic because it is unlikely to match the exit # code of other programs - if test "OK-117" != "$(test_eval_ "fail_117 && $1${LF}${LF}echo OK-\$?" 3>&1)" + test_eval_ "fail_117 && $1" + if test $? != 117 then - BUG "broken &&-chain or run-away HERE-DOC: $1" + BUG "broken &&-chain: $1" fi trace=$trace_tmp fi -- cgit v1.2.3 From cc48ddd937f9d852da73e188e4a3216cb038c421 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 30 Mar 2023 15:30:56 -0400 Subject: tests: skip test_eval_ in internal chain-lint To check for broken &&-chains, we run "fail_117 && $1" as a test snippet, and check the exit code. We use test_eval_ to do so, because that's the way we run the actual test. But we don't need any of its niceties, like "set -x" tracing. In fact, they hinder us, because we have to explicitly disable them. So let's skip that and use "eval" more directly, which is simpler. I had hoped it would also be faster, but it doesn't seem to produce a measurable improvement (probably because it's just running internal shell commands, with no subshells or forks). Note that there is one gotcha: even though we don't intend to run any of the commands if the &&-chain is intact, an error like this: test_expect_success 'broken' ' # this next line breaks the &&-chain true # and then this one is executed even by the linter return 1 ' means we'll "return 1" from the eval, and thus from test_run_(). We actually do notice this in test_expect_success, but only by saying "hey, this test didn't say it was OK, so it must have failed", which is not right (it should say "broken &&-chain"). We can handle this by calling test_eval_inner_() instead, which is our trick for wrapping "return" in a test snippet. But to do that, we have to push the trace code out of that inner function and into test_eval_(). This is arguably where it belonged in the first place, but it never mattered because the "inner_" function had only one caller. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/test-lib.sh | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/t/test-lib.sh b/t/test-lib.sh index 0048ec7b6f..293caf0f20 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -1041,10 +1041,7 @@ want_trace () { # (and we want to make sure we run any cleanup like # "set +x"). test_eval_inner_ () { - # Do not add anything extra (including LF) after '$*' - eval " - want_trace && trace_level_=$(($trace_level_+1)) && set -x - $*" + eval "$*" } test_eval_ () { @@ -1069,7 +1066,10 @@ test_eval_ () { # be _inside_ the block to avoid polluting the "set -x" output # - test_eval_inner_ "$@" &3 2>&4 + # Do not add anything extra (including LF) after '$*' + test_eval_inner_ &3 2>&4 " + want_trace && trace_level_=$(($trace_level_+1)) && set -x + $*" { test_eval_ret_=$? if want_trace @@ -1095,18 +1095,13 @@ test_run_ () { expecting_failure=$2 if test "${GIT_TEST_CHAIN_LINT:-1}" != 0; then - # turn off tracing for this test-eval, as it simply creates - # confusing noise in the "-x" output - trace_tmp=$trace - trace= # 117 is magic because it is unlikely to match the exit # code of other programs - test_eval_ "fail_117 && $1" + test_eval_inner_ "fail_117 && $1" &3 2>&4 if test $? != 117 then BUG "broken &&-chain: $1" fi - trace=$trace_tmp fi setup_malloc_check -- cgit v1.2.3