Merge pull request #2783 from webmin/dev/miniserv-helpers

Fix to extract miniserv helpers into library

makedist.pl

@@ -27,7 +27,8 @@ $vers || usage();
 
 @files = ("config.cgi", "config-*-linux",
 	  "config-solaris", "images", "index.cgi", "mime.types",
-	  "miniserv.pl", "os_list.txt", "perlpath.pl", "setup.sh", "setup.pl", "setup.bat",
+	  "miniserv.pl", "miniserv-lib.pl", "os_list.txt", "perlpath.pl",
+	  "setup.sh", "setup.pl", "setup.bat",
 	  "setup-repos.sh", "version", "web-lib.pl", "web-lib-funcs.pl",
 	  "config_save.cgi", "chooser.cgi", "miniserv.pem",
 	  "config-aix", "update-from-repo.sh", "README.md",

t/README.md

@@ -33,37 +33,19 @@ You need Devel::Cover installed.
 | File | What it checks |
 | --- | --- |
 | `compile.t` | Every `.pl`, `.cgi`, and shebang-perl script in `bin/` parses cleanly (`perl -c`). Catches breakage from bulk refactors without browsing every page. ~12s for the full tree. |
-| `miniserv.t` | Contract test for `miniserv.pl` functions — status codes, headers, body rendering, log behaviour. Demonstrates the require-and-stub pattern below. |
+| `miniserv.t` | Contract test for `miniserv-lib.pl` helpers used by `miniserv.pl` — status codes, headers, body rendering, log behaviour. |
 
-## The require-and-stub pattern
+## Testing helpers from executable code
 
-Many Webmin scripts mix sub definitions with a main body that opens sockets,
-reads `/etc/webmin/*`, or spawns CGIs. To test individual subs in isolation
-we need to `require` the script as a library without running the main body.
+Many Webmin scripts mix helper definitions with a main body that opens sockets,
+reads `/etc/webmin/*`, or spawns CGIs. Prefer moving testable helpers into a
+small `*-lib.pl` file, keeping the executable script focused on startup and
+request handling, and requiring the library from tests. `miniserv.pl` follows
+that pattern: daemon startup remains in `miniserv.pl`, while helper coverage
+loads `miniserv-lib.pl` directly.
 
-Two complementary idioms can be used. Both work; they look different because
-the underlying script does.
-
-### Block wrap (main body precedes sub definitions)
-
-Used by `miniserv.pl`. The executable preamble runs at file scope (so any 
-`my` vars stay file-scoped for the subs below); the main body wraps in
-`unless (caller)`:
-
-```perl
-#!/usr/bin/perl
-use Foo;            # use lines and pragmas stay outside the guard
-
-unless (caller) {
-
-# main body: arg parsing, setup, the actual work
-...
-
-} # end of unless (caller)
-
-sub helper { ... }
-sub other_helper { ... }
-```
+A second useful idiom still applies to simple CLI tools whose sub definitions
+precede the main call.
 
 ### One-liner (sub definitions precede the main call)
 
@@ -90,7 +72,7 @@ under `require`.
 
 `miniserv.t` is the canonical example. The pattern:
 
-1. `require` the script. The guard skips its main body.
+1. `require` the helper library.
 2. Replace side-effecting subs (socket I/O, logging, disk reads) with
    capture-buffer overrides under `no warnings 'redefine'`.
 3. Populate package globals (`%miniserv::config`, `@miniserv::roots`, etc.)

t/compile.t

@@ -24,7 +24,7 @@ use Test::More;
 use File::Find;
 use File::Basename qw(dirname);
 use File::Spec;
-use Cwd qw(abs_path);
+use Cwd qw(abs_path getcwd);
 
 my $root = abs_path(File::Spec->catdir(dirname(__FILE__), '..'));
 chdir($root) or die "chdir($root): $!";
@@ -68,6 +68,22 @@ if ($filter) {
 	@files or do { diag("filter '$filter' matched zero files"); plan skip_all => "no files match filter"; };
 	}
 
+if (grep { $_ eq q{./miniserv.pl} } @files) {
+	my $cwd = getcwd();
+	my $tmpdir = File::Spec->tmpdir();
+	my $miniserv = File::Spec->catfile($root, q{miniserv.pl});
+	chdir($tmpdir) or BAIL_OUT("chdir($tmpdir): $!");
+	my $out = qx{perl -c -- "$miniserv" 2>&1};
+	chdir($cwd) or BAIL_OUT("chdir($cwd): $!");
+	if ($out =~ /\bsyntax OK\b/) {
+		pass(q{miniserv.pl compiles outside the source tree});
+		}
+	else {
+		fail(q{miniserv.pl compiles outside the source tree});
+		diag($out);
+		}
+	}
+
 diag("compile-checking " . scalar(@files) . " files");
 
 for my $f (@files) {

t/miniserv.t

@@ -1,11 +1,10 @@
 #!/usr/bin/perl
-# Unit tests for miniserv.pl helper subs.
+# Unit tests for miniserv helper subs.
 #
-# miniserv.pl is loaded as a module; its top-level script body is skipped
-# by the `unless (caller) { ... }` guard, so we only get the sub
-# definitions plus a handful of pure-constant globals (@itoa64, @weekday,
-# @month, @miniserv_argv). Everything else (%config, @roots, $datestr,
-# etc.) we populate ourselves.
+# miniserv-lib.pl contains the helper functions and pure-constant globals
+# used by miniserv.pl. The daemon startup path stays in miniserv.pl, while
+# tests load this library directly and populate mutable runtime globals
+# (%config, @roots, $datestr, etc.) themselves.
 #
 # Each helper has its own `subtest`. Assertions target the contract
 # (status code present, headers present, redaction happened, round-trip
@@ -18,9 +17,9 @@ use Test::More;
 use File::Basename qw(dirname);
 use File::Spec;
 
-my $script = File::Spec->rel2abs(
-	File::Spec->catfile(dirname(__FILE__), '..', 'miniserv.pl'));
-require $script;
+my $lib = File::Spec->rel2abs(
+	File::Spec->catfile(dirname(__FILE__), '..', 'miniserv-lib.pl'));
+require $lib;
 
 # Capture buffers populated by overridden I/O subs.
 our @written;
@@ -652,10 +651,9 @@ subtest 'ssl_hostname_match' => sub {
 	   ':port suffix is stripped before matching');
 };
 
-# Capability probes used by the crypto subtests below. miniserv.pl's
-# detection logic runs inside its `unless(caller)` block, so when we load
-# it as a module the $use_md5 / $use_sha512 / $use_hmac_sha256 globals are
-# undef. Recreate them here from the same probes miniserv.pl uses itself.
+# Capability probes used by the crypto subtests below. miniserv.pl sets
+# these globals during daemon startup, while the tests load only the helper
+# library. Recreate them here from the same probes miniserv.pl uses itself.
 my $have_md5 = eval {
 	require Digest::MD5;
 	Digest::MD5->new->add('x');