[PATCH 0/5] tiny test overhead reductions

[PATCH 0/5] tiny test overhead reductions

[Eric Wong]

Getting rid of IPC::Run for running 3rd party programs is a tiny
stope towards improving ease-of-testing for new contributors.

Next step might be be enabling mod_perl-like functionality for
our own short-lived scripts to speed up startup time...

Finally, we can eliminate daemon worker processes from most
tests (and explicitly test it in one place) since we don't
need to repeat ourselves.

Eric Wong (5):
  t/*.t: remove IPC::Run dependency for git commands
  t/httpd-corner.t: drop unnecessary bytes:: for length()
  t/httpd-corner.t: get rid of IPC::Run for running curl
  t/hl_mod.t: remove IPC::Run (and File::Temp) dependency
  t/*.t: disable nntpd/httpd worker processes in most tests

 t/git.t             | 12 ++++++------
 t/hl_mod.t          | 24 ++++++++++++++++--------
 t/httpd-corner.psgi |  3 +++
 t/httpd-corner.t    | 44 +++++++++++++++++++++++++++++---------------
 t/httpd.t           |  2 +-
 t/v2mirror.t        |  3 ++-
 t/v2writable.t      |  2 +-
 t/www_listing.t     | 13 +++++++------
 8 files changed, 65 insertions(+), 38 deletions(-)

[PATCH 1/5] t/*.t: remove IPC::Run dependency for git commands

[Eric Wong]

One small step towards making tests easier-to-run.  We can rely
on "local $ENV{GIT_DIR}" for potentially shell-unsafe path
names, and the rest of our path names are relative and don't
contain characters which require escaping.
---
 t/git.t         | 12 ++++++------
 t/www_listing.t | 11 ++++++-----
 2 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/t/git.t b/t/git.t
index 445ef248..a496f851 100644
--- a/t/git.t
+++ b/t/git.t
@@ -7,16 +7,14 @@ use File::Temp qw/tempdir/;
 my $dir = tempdir('pi-git-XXXXXX', TMPDIR => 1, CLEANUP => 1);
 use PublicInbox::Spawn qw(popen_rd);
 
-eval { require IPC::Run } or plan skip_all => 'IPC::Run missing';
 use_ok 'PublicInbox::Git';
 
 {
 	is(system(qw(git init -q --bare), $dir), 0, 'created git directory');
-	my $cmd = [ 'git', "--git-dir=$dir", 'fast-import', '--quiet' ];
-
 	my $fi_data = './t/git.fast-import-data';
 	ok(-r $fi_data, "fast-import data readable (or run test at top level)");
-	IPC::Run::run($cmd, '<', $fi_data);
+	local $ENV{GIT_DIR} = $dir;
+	system("git fast-import --quiet <$fi_data");
 	is($?, 0, 'fast-import succeeded');
 }
 
@@ -46,8 +44,10 @@ if (1) {
 	my $size = -s $big_data;
 	ok($size > 8192, 'file is big enough');
 
-	my $buf = '';
-	IPC::Run::run($cmd, '<', $big_data, '>', \$buf);
+	my $buf = do {
+		local $ENV{GIT_DIR} = $dir;
+		`git hash-object -w --stdin <$big_data`;
+	};
 	is(0, $?, 'hashed object successfully');
 	chomp $buf;
 
diff --git a/t/www_listing.t b/t/www_listing.t
index 9f71257d..3d1b64ad 100644
--- a/t/www_listing.t
+++ b/t/www_listing.t
@@ -7,7 +7,7 @@ use Test::More;
 use PublicInbox::Spawn qw(which);
 use File::Temp qw/tempdir/;
 require './t/common.perl';
-my @mods = qw(URI::Escape Plack::Builder IPC::Run Digest::SHA
+my @mods = qw(URI::Escape Plack::Builder Digest::SHA
 		IO::Compress::Gzip IO::Uncompress::Gunzip HTTP::Tiny);
 foreach my $mod (@mods) {
 	eval("require $mod") or plan skip_all => "$mod missing for $0";
@@ -19,15 +19,16 @@ plan skip_all => "JSON module missing: $@" if $@;
 
 use_ok 'PublicInbox::Git';
 
-my $fi_data = './t/git.fast-import-data';
 my $tmpdir = tempdir('www_listing-tmp-XXXXXX', TMPDIR => 1, CLEANUP => 1);
 my $bare = PublicInbox::Git->new("$tmpdir/bare.git");
 is(system(qw(git init -q --bare), $bare->{git_dir}), 0, 'git init --bare');
 is(PublicInbox::WwwListing::fingerprint($bare), undef,
 	'empty repo has no fingerprint');
-
-my $cmd = [ 'git', "--git-dir=$bare->{git_dir}", qw(fast-import --quiet) ];
-ok(IPC::Run::run($cmd, '<', $fi_data), 'fast-import');
+{
+	my $fi_data = './t/git.fast-import-data';
+	local $ENV{GIT_DIR} = $bare->{git_dir};
+	is(system("git fast-import --quiet <$fi_data"), 0, 'fast-import');
+}
 
 like(PublicInbox::WwwListing::fingerprint($bare), qr/\A[a-f0-9]{40}\z/,
 	'got fingerprint with non-empty repo');

[PATCH 2/5] t/httpd-corner.t: drop unnecessary bytes:: for length()

[Eric Wong]

We don't need to force byte semantics for a buffer we clearly
create (via ->read) with byte semantics.  Since we didn't
"use bytes" in t/httpd-corner.t, it was inadvertantly made
available by IPC::Run (which goes away, next).
---
 t/httpd-corner.t | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/httpd-corner.t b/t/httpd-corner.t
index fbba4623..50aa28e3 100644
--- a/t/httpd-corner.t
+++ b/t/httpd-corner.t
@@ -144,7 +144,7 @@ SKIP: {
 	$conn->write($payload . "\r\n0\r\n\r\nGET /empty HTTP/1.0\r\n\r\n");
 	$conn->read(my $buf, 4096);
 	my $lim = 0;
-	$lim++ while ($conn->read($buf, 4096, bytes::length($buf)) && $lim < 9);
+	$lim++ while ($conn->read($buf, 4096, length($buf)) && $lim < 9);
 	my $exp = sha1_hex($payload);
 	like($buf, qr!\r\n\r\n${exp}HTTP/1\.0 200 OK\r\n!s,
 		'chunk parser can handled pipelined requests');

[PATCH 3/5] t/httpd-corner.t: get rid of IPC::Run for running curl

[Eric Wong]

We already load PublicInbox::Spawn, so there's no need to
add another dependency to make life difficult for potential
contributors.
---
 t/httpd-corner.t | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/t/httpd-corner.t b/t/httpd-corner.t
index 50aa28e3..13c0c2db 100644
--- a/t/httpd-corner.t
+++ b/t/httpd-corner.t
@@ -6,10 +6,9 @@ use strict;
 use warnings;
 use Test::More;
 use Time::HiRes qw(gettimeofday tv_interval);
-use PublicInbox::Spawn qw(which);
+use PublicInbox::Spawn qw(which spawn);
 
-foreach my $mod (qw(Plack::Util Plack::Builder
-			HTTP::Date HTTP::Status IPC::Run)) {
+foreach my $mod (qw(Plack::Util Plack::Builder HTTP::Date HTTP::Status)) {
 	eval "require $mod";
 	plan skip_all => "$mod missing for httpd-corner.t" if $@;
 }
@@ -250,19 +249,21 @@ SKIP: {
 	my ($r, $w);
 	pipe($r, $w) or die "pipe: $!";
 	my $cmd = [qw(curl --tcp-nodelay --no-buffer -T- -HExpect: -sS), $url];
-	my ($out, $err) = ('', '');
-	my $h = IPC::Run::start($cmd, $r, \$out, \$err);
-	$w->autoflush(1);
+	open my $cout, '+>', undef or die;
+	open my $cerr, '>', undef or die;
+	my $rdr = { 0 => fileno($r), 1 => fileno($cout), 2 => fileno($cerr) };
+	my $pid = spawn($cmd, undef, $rdr);
+	close $r or die "close read pipe: $!";
 	foreach my $c ('a'..'z') {
 		print $w $c or die "failed to write to curl: $!";
 		delay();
 	}
 	close $w or die "close write pipe: $!";
-	close $r or die "close read pipe: $!";
-	IPC::Run::finish($h);
+	waitpid($pid, 0);
 	is($?, 0, 'curl exited successfully');
-	is($err, '', 'no errors from curl');
-	is($out, sha1_hex($str), 'read expected body');
+	is(-s $cerr, 0, 'no errors from curl');
+	$cout->seek(0, SEEK_SET);
+	is(<$cout>, sha1_hex($str), 'read expected body');
 
 	open my $fh, '-|', qw(curl -sS), "$base/async-big" or die $!;
 	my $n = 0;

[PATCH 4/5] t/hl_mod.t: remove IPC::Run (and File::Temp) dependency

[Eric Wong]

We already load PublicInbox::Spawn for which(), so using spawn()
isn't unreasonable.  And rely on "skip" to log the omitted test
if w3m is missing, which means we need to update the "&&"
escaping test to be self-referential on the same line.

File::Temp was totally unused, there; and we can use "open ...,undef"
in Perl to easily create anonymous temporary files for use with
spawn().
---
 t/hl_mod.t | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/t/hl_mod.t b/t/hl_mod.t
index 52ef39dc..4942404c 100644
--- a/t/hl_mod.t
+++ b/t/hl_mod.t
@@ -4,8 +4,10 @@
 use strict;
 use warnings;
 use Test::More;
+use PublicInbox::Spawn qw(which spawn);
+use Fcntl qw(:seek);
 eval { require highlight } or
-	plan skip_all => 'failed to load highlight.pm';
+	plan skip_all => "failed to load highlight.pm for $0";
 use_ok 'PublicInbox::HlMod';
 my $hls = PublicInbox::HlMod->new;
 ok($hls, 'initialized OK');
@@ -21,20 +23,26 @@ my $orig = $str;
 	is(ref($ref), 'SCALAR', 'got a scalar reference back');
 	ok(utf8::valid($$ref), 'resulting string is utf8::valid');
 	like($$ref, qr/I can see you!/, 'we can see ourselves in output');
-	like($$ref, qr/&&/, 'escaped');
+	like($$ref, qr/&&/, 'escaped &&');
 	my $lref = $hls->do_hl_lang(\$str, 'perl');
 	is($$ref, $$lref, 'do_hl_lang matches do_hl');
 
-	use PublicInbox::Spawn qw(which);
-	if (eval { require IPC::Run } && which('w3m')) {
-		require File::Temp;
+	SKIP: {
+		which('w3m') or skip 'w3m(1) missing to check output', 1;
 		my $cmd = [ qw(w3m -T text/html -dump -config /dev/null) ];
-		my ($out, $err) = ('', '');
-
-		IPC::Run::run($cmd, \('<pre>'.$$ref.'</pre>'), \$out, \$err);
+		open my $in, '+>', undef or die;
+		open my $out, '+>', undef or die;
+		my $rdr = { 0 => fileno($in), 1 => fileno($out) };
+		$in->autoflush(1);
+		print $in '<pre>', $$ref, '</pre>' or die;
+		$in->seek(0, SEEK_SET) or die;
+		my $pid = spawn($cmd, undef, $rdr);
+		waitpid($pid, 0);
 		# expand tabs and normalize whitespace,
 		# w3m doesn't preserve tabs
 		$orig =~ s/\t/        /gs;
+		$out->seek(0, SEEK_SET) or die;
+		$out = do { local $/; <$out> };
 		$out =~ s/\s*\z//sg;
 		$orig =~ s/\s*\z//sg;
 		is($out, $orig, 'w3m output matches');

[PATCH 5/5] t/*.t: disable nntpd/httpd worker processes in most tests

[Eric Wong]

And explicitly test for respawning in t/httpd-corner.t

There's no need to have an extra entries in the process table
for most tests we run, since that's not what we're testing.
---
 t/httpd-corner.psgi |  3 +++
 t/httpd-corner.t    | 21 +++++++++++++++++----
 t/httpd.t           |  2 +-
 t/v2mirror.t        |  3 ++-
 t/v2writable.t      |  2 +-
 t/www_listing.t     |  2 +-
 6 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/t/httpd-corner.psgi b/t/httpd-corner.psgi
index 5628f4ab..bf38d1ff 100644
--- a/t/httpd-corner.psgi
+++ b/t/httpd-corner.psgi
@@ -85,6 +85,9 @@ my $app = sub {
 			close $null;
 			[ 200, [ qw(Content-Type application/octet-stream) ]];
 		});
+	} elsif ($path eq '/pid') {
+		$code = 200;
+		push @$body, $$;
 	}
 
 	[ $code, $h, $body ]
diff --git a/t/httpd-corner.t b/t/httpd-corner.t
index 13c0c2db..75573c3e 100644
--- a/t/httpd-corner.t
+++ b/t/httpd-corner.t
@@ -28,7 +28,7 @@ my $err = "$tmpdir/stderr.log";
 my $out = "$tmpdir/stdout.log";
 my $httpd = 'blib/script/public-inbox-httpd';
 my $psgi = "./t/httpd-corner.psgi";
-my $sock = tcp_server();
+my $sock = tcp_server() or die;
 
 # make sure stdin is not a pipe for lsof test to check for leaking pipes
 open(STDIN, '<', '/dev/null') or die 'no /dev/null: $!';
@@ -63,9 +63,22 @@ my $spawn_httpd = sub {
 	ok(defined $pid, 'forked httpd process successfully');
 };
 
-{
-	ok($sock, 'sock created');
-	$spawn_httpd->('-W0');
+$spawn_httpd->();
+if ('test worker death') {
+	my $conn = conn_for($sock, 'killed worker');
+	$conn->write("GET /pid HTTP/1.0\r\n\r\n");
+	ok($conn->read(my $buf, 8192), 'read response');
+	my ($head, $body) = split(/\r\n\r\n/, $buf);
+	like($body, qr/\A[0-9]+\z/, '/pid response');
+	my $pid = $body;
+	is(kill('KILL', $pid), 1, 'killed worker');
+
+	$conn = conn_for($sock, 'respawned worker');
+	$conn->write("GET /pid HTTP/1.0\r\n\r\n");
+	ok($conn->read($buf, 8192), 'read response');
+	($head, $body) = split(/\r\n\r\n/, $buf);
+	like($body, qr/\A[0-9]+\z/, '/pid response');
+	isnt($body, $pid, 'respawned worker');
 }
 
 {
diff --git a/t/httpd.t b/t/httpd.t
index 1340a7b3..15984a78 100644
--- a/t/httpd.t
+++ b/t/httpd.t
@@ -53,7 +53,7 @@ EOF
 		$im->done($mime);
 	}
 	ok($sock, 'sock created');
-	my $cmd = [ $httpd, "--stdout=$out", "--stderr=$err" ];
+	my $cmd = [ $httpd, '-W0', "--stdout=$out", "--stderr=$err" ];
 	$pid = spawn_listener(undef, $cmd, [$sock]);
 	my $host = $sock->sockhost;
 	my $port = $sock->sockport;
diff --git a/t/v2mirror.t b/t/v2mirror.t
index a097a7f3..f826775c 100644
--- a/t/v2mirror.t
+++ b/t/v2mirror.t
@@ -66,7 +66,8 @@ END { kill 'TERM', $pid if defined $pid };
 $! = 0;
 $sock = tcp_server();
 ok($sock, 'sock created');
-my $cmd = [ "$script-httpd", "--stdout=$tmpdir/out", "--stderr=$tmpdir/err" ];
+my $httpd = "$script-httpd";
+my $cmd = [ $httpd, '-W0', "--stdout=$tmpdir/out", "--stderr=$tmpdir/err" ];
 ok(defined($pid = spawn_listener(undef, $cmd, [ $sock ])),
 	'spawned httpd process successfully');
 my ($host, $port) = ($sock->sockhost, $sock->sockport);
diff --git a/t/v2writable.t b/t/v2writable.t
index bfe17d0a..28420bb9 100644
--- a/t/v2writable.t
+++ b/t/v2writable.t
@@ -167,7 +167,7 @@ EOF
 	my $len;
 	END { kill 'TERM', $pid if defined $pid };
 	my $nntpd = 'blib/script/public-inbox-nntpd';
-	my $cmd = [ $nntpd, "--stdout=$out", "--stderr=$err" ];
+	my $cmd = [ $nntpd, '-W0', "--stdout=$out", "--stderr=$err" ];
 	$pid = spawn_listener({ PI_CONFIG => $pi_config }, $cmd, [ $sock ]);
 	my $host_port = $sock->sockhost . ':' . $sock->sockport;
 	my $n = Net::NNTP->new($host_port);
diff --git a/t/www_listing.t b/t/www_listing.t
index 3d1b64ad..61a059e5 100644
--- a/t/www_listing.t
+++ b/t/www_listing.t
@@ -106,7 +106,7 @@ SKIP: {
 
 	close $fh or die;
 	my $env = { PI_CONFIG => $cfgfile };
-	my $cmd = [ $httpd, "--stdout=$out", "--stderr=$err" ];
+	my $cmd = [ $httpd, '-W0', "--stdout=$out", "--stderr=$err" ];
 	$pid = spawn_listener($env, $cmd, [$sock]);
 	$sock = undef;