chainlint.pl: recognize test bodies defined via heredoc

Commit Message

Eric Sunshine July 2, 2024, 11:50 p.m. UTC

From: Eric Sunshine <[e-mail]>

In order to check tests for semantic problems, chainlint.pl scans test
scripts, looking for tests defined as:

    test_expect_success [prereq] title '
        body
    '

where `body` is a single string which is then treated as a standalone
chunk of code and "linted" to detect semantic issues. (The same happens
for `test_expect_failure` definitions.)

The introduction of test definitions in which the test body is instead
presented via a heredoc rather than as a single string creates a blind
spot in the linting process since such invocations are not recognized by
chainlint.pl.

Address this shortcoming by also recognizing tests defined as:

    test_expect_success [prereq] title - <<\EOT
        body
    EOT

A minor complication is that chainlint.pl has never considered heredoc
bodies significant since it doesn't scan them for semantic problems,
thus it has always simply thrown them away. However, with the new
`test_expect_success` calling sequence, heredoc bodies become
meaningful, thus need to be captured.

Signed-off-by: Eric Sunshine <[e-mail]>
---

This is a clean-room implementation which serves the same purpose as a
change proposed[1] by Peff; it was created before I looked at Peff's
proposal. The two independent implementations turned out quite similar,
but the one implemented by this patch takes a more formal and paranoid
stance. In particular, unlike Peff's patch, it doesn't trust that the
most-recently-seen heredoc body is one associated with the
`test_expect_success` invocation.

This patch can sit either at the top or bottom of Peff's series[2].

There was also related discussion of improving the chainlint self-test
infrastructure[3], however, such proposed changes needn't hold up Peff's
series[2]; such improvements can be applied after the dust settles. On
the other hand, Peff, if you plan to reroll for some reason, feel free
to incorporate this patch into your series.

[1]: https://lore.kernel.org/git/[e-mail]/
[2]: https://lore.kernel.org/git/[e-mail]/
[3]: https://lore.kernel.org/git/[e-mail]/

 t/chainlint.pl | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

Patch

diff --git a/t/chainlint.pl b/t/chainlint.pl
index 1bbd985b78..eba509b8e1 100755
--- a/t/chainlint.pl
+++ b/t/chainlint.pl
@@ -174,6 +174,8 @@  sub swallow_heredocs {
 		$$b =~ /(?:\G|\n)$indent\Q$$tag[0]\E(?:\n|\z)/gc;
 		if (pos($$b) > $start) {
 			my $body = substr($$b, $start, pos($$b) - $start);
+			$self->{parser}->{heredocs}->{$$tag[0]} =
+			    substr($body, 0, length($body) - length($&));
 			$self->{lineno} += () = $body =~ /\n/sg;
 			next;
 		}
@@ -232,7 +234,8 @@  sub new {
 	my $self = bless {
 		buff => [],
 		stop => [],
-		output => []
+		output => [],
+		heredocs => {},
 	} => $class;
 	$self->{lexer} = Lexer->new($self, $s);
 	return $self;
@@ -616,7 +619,9 @@  sub unwrap {
 
 sub check_test {
 	my $self = shift @_;
-	my ($title, $body) = map(unwrap, @_);
+	my $title = unwrap(shift @_);
+	my $body = unwrap(shift @_);
+	$body = shift @_ if $body eq '-';
 	$self->{ntests}++;
 	my $parser = TestParser->new(\$body);
 	my @tokens = $parser->parse();
@@ -649,8 +654,13 @@  sub parse_cmd {
 	return @tokens unless @tokens && $tokens[0]->[0] =~ /^test_expect_(?:success|failure)$/;
 	my $n = $#tokens;
 	$n-- while $n >= 0 && $tokens[$n]->[0] =~ /^(?:[;&\n|]|&&|\|\|)$/;
-	$self->check_test($tokens[1], $tokens[2]) if $n == 2; # title body
-	$self->check_test($tokens[2], $tokens[3]) if $n > 2;  # prereq title body
+	my $herebody;
+	if ($n >= 2 && $tokens[$n-1]->[0] eq '-' && $tokens[$n]->[0] =~ /^<<-?(.+)$/) {
+		$herebody = $self->{heredocs}->{$1};
+		$n--;
+	}
+	$self->check_test($tokens[1], $tokens[2], $herebody) if $n == 2; # title body
+	$self->check_test($tokens[2], $tokens[3], $herebody) if $n > 2;  # prereq title body
 	return @tokens;
 }