[PATCH 0/6] t/cgi.t: test speedups and cleanups

[PATCH 0/6] t/cgi.t: test speedups and cleanups

[Eric Wong]

While we relied on CGI.pm early in the project, we've moved onto PSGI
and relied on PSGI for providing CGI compatibility.  Thus, most of our
CGI-based tests are redundant and Perl startup overhead is painful.

On my system, this cuts the t/cgi.t from 4.4s to 1.8s for a nice
improvement.  Moving some tests to t/plack.t did not increase runtimes
in a meaningful way.

Eric Wong (6):
  t/cgi.t: eliminate some cruft and unnecessary tests
  t/cgi.t: remove redundant redirect check
  t/cgi.t: remove atom.xml test
  t/cgi.t: move dumb HTTP git clone/fetch tests to plack.t
  t/cgi.t: move expected failure tests to t/plack.t
  t/cgi.t: remove more redundant tests

 t/cgi.t   | 105 ++++++++----------------------------------------------
 t/plack.t |  43 ++++++++++++++++++++--
 2 files changed, 54 insertions(+), 94 deletions(-)

-- 
EW

[PATCH 1/6] t/cgi.t: eliminate some cruft and unnecessary tests

[Eric Wong]

More of this test will be, we use PSGI nowadays; and
most of these tests can be ported over to use PSGI and
not fork+exec as much.
---
 t/cgi.t | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/t/cgi.t b/t/cgi.t
index d92749b..ac2c69f 100644
--- a/t/cgi.t
+++ b/t/cgi.t
@@ -48,9 +48,8 @@ my $im = PublicInbox::Import->new($git, 'test', $addr);
 {
 	local $ENV{HOME} = $home;
 
-	# ensure successful message delivery
-	{
-		my $mime = Email::MIME->new(<<EOF);
+	# inject some messages:
+	my $mime = Email::MIME->new(<<EOF);
 From: Me <me\@example.com>
 To: You <you\@example.com>
 Cc: $addr
@@ -60,15 +59,10 @@ Date: Thu, 01 Jan 1970 00:00:00 +0000
 
 zzzzzz
 EOF
-		$im->add($mime);
-		$im->done;
-		my $rev = `git --git-dir=$maindir rev-list HEAD`;
-		like($rev, qr/\A[a-f0-9]{40}/, "good revision committed");
-	}
+	$im->add($mime);
 
 	# deliver a reply, too
-	{
-		my $reply = Email::MIME->new(<<EOF);
+	my $reply = Email::MIME->new(<<EOF);
 From: You <you\@example.com>
 To: Me <me\@example.com>
 Cc: $addr
@@ -82,12 +76,8 @@ Me wrote:
 
 what?
 EOF
-		$im->add($reply);
-		$im->done;
-		my $rev = `git --git-dir=$maindir rev-list HEAD`;
-		like($rev, qr/\A[a-f0-9]{40}/, "good revision committed");
-	}
-
+	$im->add($reply);
+	$im->done;
 }
 
 # obvious failures, first
@@ -118,7 +108,7 @@ EOF
 	like($res->{head}, qr/Status:\s*206/i, "info/refs partial past end OK");
 	is($res->{body}, substr($orig, 5), 'partial body OK past end');
 }
-use Data::Dumper;
+
 # atom feeds
 {
 	local $ENV{HOME} = $home;
-- 
EW

[PATCH 2/6] t/cgi.t: remove redundant redirect check

[Eric Wong]

t/plack.t already has the same test.
---
 t/cgi.t   | 8 --------
 t/plack.t | 3 ++-
 2 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/t/cgi.t b/t/cgi.t
index ac2c69f..06519c3 100644
--- a/t/cgi.t
+++ b/t/cgi.t
@@ -195,14 +195,6 @@ EOF
 	}
 }
 
-# redirect list-name-only URLs
-{
-	local $ENV{HOME} = $home;
-	my $res = cgi_run("/test");
-	like($res->{head}, qr/Status: 301 Moved/, "redirected status");
-	like($res->{head}, qr!/test/!, "redirected with slash");
-}
-
 done_testing();
 
 sub run_with_env {
diff --git a/t/plack.t b/t/plack.t
index 7eb7d7f..70cd20e 100644
--- a/t/plack.t
+++ b/t/plack.t
@@ -92,7 +92,8 @@ EOF
 		my $to = "$from/";
 		my $res = $cb->(GET($from));
 		is(301, $res->code, 'is permanent redirect');
-		is($to, $res->header('Location'), 'redirect location matches');
+		is($to, $res->header('Location'),
+			'redirect location matches with trailing slash');
 	});
 
 	my $pfx = 'http://example.com/test';
-- 
EW

[PATCH 3/6] t/cgi.t: remove atom.xml test

[Eric Wong]

It is redundant with what is in t/plack.t
---
 t/cgi.t   | 12 ------------
 t/plack.t |  7 +++++--
 2 files changed, 5 insertions(+), 14 deletions(-)

diff --git a/t/cgi.t b/t/cgi.t
index 06519c3..382c21f 100644
--- a/t/cgi.t
+++ b/t/cgi.t
@@ -109,18 +109,6 @@ EOF
 	is($res->{body}, substr($orig, 5), 'partial body OK past end');
 }
 
-# atom feeds
-{
-	local $ENV{HOME} = $home;
-	my $res = cgi_run("/test/atom.xml");
-	like($res->{body}, qr/<title>test for public-inbox/,
-		"set title in XML feed");
-	like($res->{body},
-		qr!http://test\.example\.com/test/blah\@example\.com/!,
-		"link id set");
-	like($res->{body}, qr/what\?/, "reply included");
-}
-
 # message-id pages
 {
 	local $ENV{HOME} = $home;
diff --git a/t/plack.t b/t/plack.t
index 70cd20e..1a719b4 100644
--- a/t/plack.t
+++ b/t/plack.t
@@ -135,9 +135,12 @@ EOF
 		my ($cb) = @_;
 		my $res = $cb->(GET($pfx . '/atom.xml'));
 		is(200, $res->code, 'success response received for atom');
-		like($res->content,
-			qr!link\s+href="\Q$pfx\E/blah\@example\.com/"!s,
+		my $body = $res->content;
+		like($body, qr!link\s+href="\Q$pfx\E/blah\@example\.com/"!s,
 			'atom feed generated correct URL');
+		like($body, qr/<title>test for public-inbox/,
+			"set title in XML feed");
+		like($body, qr/zzzzzz/, 'body included');
 	});
 
 	test_psgi($app, sub {
-- 
EW

[PATCH 4/6] t/cgi.t: move dumb HTTP git clone/fetch tests to plack.t

[Eric Wong]

No need to test this via CGI .cgi is a wrapper around
PSGI and PSGI tests are way faster.
---
 t/cgi.t   | 19 -------------------
 t/plack.t | 20 ++++++++++++++++++++
 2 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/t/cgi.t b/t/cgi.t
index 382c21f..a25d2ee 100644
--- a/t/cgi.t
+++ b/t/cgi.t
@@ -90,25 +90,6 @@ EOF
 	like($res->{head}, qr/Status:\s*404/i, "index returns 404");
 }
 
-# dumb HTTP support
-{
-	local $ENV{HOME} = $home;
-	my $path = "/test/info/refs";
-	my $res = cgi_run($path);
-	like($res->{head}, qr/Status:\s*200/i, "info/refs readable");
-	my $orig = $res->{body};
-
-	local $ENV{HTTP_RANGE} = 'bytes=5-10';
-	$res = cgi_run($path);
-	like($res->{head}, qr/Status:\s*206/i, "info/refs partial OK");
-	is($res->{body}, substr($orig, 5, 6), 'partial body OK');
-
-	local $ENV{HTTP_RANGE} = 'bytes=5-';
-	$res = cgi_run($path);
-	like($res->{head}, qr/Status:\s*206/i, "info/refs partial past end OK");
-	is($res->{body}, substr($orig, 5), 'partial body OK past end');
-}
-
 # message-id pages
 {
 	local $ENV{HOME} = $home;
diff --git a/t/plack.t b/t/plack.t
index 1a719b4..14c9b65 100644
--- a/t/plack.t
+++ b/t/plack.t
@@ -220,6 +220,26 @@ EOF
 				'redirect from x40 MIDs works');
 		}
 	});
+
+	# dumb HTTP clone/fetch support
+	test_psgi($app, sub {
+		my ($cb) = @_;
+		my $path = '/test/info/refs';
+		my $req = HTTP::Request->new('GET' => $path);
+		my $res = $cb->($req);
+		is(200, $res->code, 'refs readable');
+		my $orig = $res->content;
+
+		$req->header('Range', 'bytes=5-10');
+		$res = $cb->($req);
+		is(206, $res->code, 'got partial response');
+		is($res->content, substr($orig, 5, 6), 'partial body OK');
+
+		$req->header('Range', 'bytes=5-');
+		$res = $cb->($req);
+		is(206, $res->code, 'got partial another response');
+		is($res->content, substr($orig, 5), 'partial body OK past end');
+	});
 }
 
 done_testing();
-- 
EW

[PATCH 5/6] t/cgi.t: move expected failure tests to t/plack.t

[Eric Wong]

No point in implementing these slowly with the CGI wrapper
when PSGI is sufficient for testing.
---
 t/cgi.t   | 10 ----------
 t/plack.t | 13 +++++++++++++
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/t/cgi.t b/t/cgi.t
index a25d2ee..e705cd7 100644
--- a/t/cgi.t
+++ b/t/cgi.t
@@ -80,16 +80,6 @@ EOF
 	$im->done;
 }
 
-# obvious failures, first
-{
-	local $ENV{HOME} = $home;
-	my $res = cgi_run("/", "", "PUT");
-	like($res->{head}, qr/Status:\s*405/i, "PUT not allowed");
-
-	$res = cgi_run("/");
-	like($res->{head}, qr/Status:\s*404/i, "index returns 404");
-}
-
 # message-id pages
 {
 	local $ENV{HOME} = $home;
diff --git a/t/plack.t b/t/plack.t
index 14c9b65..9901186 100644
--- a/t/plack.t
+++ b/t/plack.t
@@ -240,6 +240,19 @@ EOF
 		is(206, $res->code, 'got partial another response');
 		is($res->content, substr($orig, 5), 'partial body OK past end');
 	});
+
+	# things which should fail
+	test_psgi($app, sub {
+		my ($cb) = @_;
+
+		my $res = $cb->(PUT('/'));
+		is(405, $res->code, 'no PUT to / allowed');
+		$res = $cb->(PUT('/test/'));
+		is(405, $res->code, 'no PUT /$INBOX allowed');
+
+		# TODO
+		# $res = $cb->(GET('/'));
+	});
 }
 
 done_testing();
-- 
EW

[PATCH 6/6] t/cgi.t: remove more redundant tests

[Eric Wong]

Most of these test cases are in t/plack.t, already; and that
runs much faster.  Just ensure the slashy corner case and search
stuff works.  While we're at it, avoid using the
public-inbox-index command and just use the internal API to
index.
---
 t/cgi.t | 36 +++++++++---------------------------
 1 file changed, 9 insertions(+), 27 deletions(-)

diff --git a/t/cgi.t b/t/cgi.t
index e705cd7..aedd79e 100644
--- a/t/cgi.t
+++ b/t/cgi.t
@@ -12,7 +12,6 @@ eval { require IPC::Run };
 plan skip_all => "missing IPC::Run for t/cgi.t" if $@;
 
 use constant CGI => "blib/script/public-inbox.cgi";
-my $index = "blib/script/public-inbox-index";
 my $tmpdir = tempdir('pi-cgi-XXXXXX', TMPDIR => 1, CLEANUP => 1);
 my $home = "$tmpdir/pi-home";
 my $pi_home = "$home/.public-inbox";
@@ -77,14 +76,9 @@ Me wrote:
 what?
 EOF
 	$im->add($reply);
-	$im->done;
-}
 
-# message-id pages
-{
-	local $ENV{HOME} = $home;
 	my $slashy_mid = 'slashy/asdf@example.com';
-	my $reply = Email::MIME->new(<<EOF);
+	my $slashy = Email::MIME->new(<<EOF);
 From: You <you\@example.com>
 To: Me <me\@example.com>
 Cc: $addr
@@ -94,30 +88,12 @@ Date: Thu, 01 Jan 1970 00:00:01 +0000
 
 slashy
 EOF
-	$im->add($reply);
+	$im->add($slashy);
 	$im->done;
 
 	my $res = cgi_run("/test/slashy/asdf\@example.com/raw");
 	like($res->{body}, qr/Message-Id: <\Q$slashy_mid\E>/,
 		"slashy mid raw hit");
-
-	$res = cgi_run("/test/blahblah\@example.com/raw");
-	like($res->{body}, qr/Message-Id: <blahblah\@example\.com>/,
-		"mid raw hit");
-
-	$res = cgi_run("/test/blahblah\@example.com/");
-	like($res->{body}, qr/\A<html>/, "mid html hit");
-	like($res->{head}, qr/Status: 200 OK/, "200 response");
-
-	$res = cgi_run("/test/blahblah\@example.com/f/");
-	like($res->{head}, qr/Status: 301 Moved/, "301 response");
-	like($res->{head},
-		qr!^Location: http://[^/]+/test/blahblah\@example\.com/\r\n!ms,
-		'301 redirect location');
-
-	$res = cgi_run("/test/new.html");
-	like($res->{body}, qr/slashy%2Fasdf\@example\.com/,
-		"slashy URL generated correctly");
 }
 
 # retrieve thread as an mbox
@@ -126,7 +102,13 @@ EOF
 	my $path = "/test/blahblah\@example.com/t.mbox.gz";
 	my $res = cgi_run($path);
 	like($res->{head}, qr/^Status: 501 /, "search not-yet-enabled");
-	my $indexed = system($index, $maindir) == 0;
+	my $indexed;
+	eval {
+		require PublicInbox::SearchIdx;
+		my $s = PublicInbox::SearchIdx->new($maindir, 1);
+		$s->index_sync;
+		$indexed = 1;
+	};
 	if ($indexed) {
 		$res = cgi_run($path);
 		like($res->{head}, qr/^Status: 200 /, "search returned mbox");
-- 
EW