Merge pull request #2715 from swelljoe/test-all-prs

Test all PRs

.github/workflows/tests.yml

@@ -0,0 +1,19 @@
+name: Tests
+
+on:
+  pull_request:
+    branches:
+      - master
+  push:
+    branches:
+      - master
+
+jobs:
+  prove:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+      - name: Install Perl::Critic
+        run: sudo apt-get update && sudo apt-get install -y libperl-critic-perl
+      - name: prove -lr
+        run: prove -lr

t/README.md

@@ -0,0 +1,148 @@
+# Webmin test suite
+
+Two kinds of tests live under this tree:
+
+- Repo-root `t/` for core code (`miniserv.pl`, `web-lib-funcs.pl`, top-level
+  scripts, `bin/`).
+- Per-module `t/` for module-internal coverage (see `nftables/t/` for the
+  established pattern: `perlcritic.t` plus a `run-tests.t` smoke harness).
+
+## Running tests
+
+```sh
+prove -lr                                # everything, including modules t/
+prove -lr t                              # everything under repo-root t/
+prove t/compile.t                        # one test file
+WEBMIN_COMPILE_T_FILTER='^\./acl/' prove t/compile.t   # one module
+```
+
+`prove` and Test::More are core, though on RPM-based distros, you need
+`perl-Test-Harness`.
+
+## Coverage reports
+
+```sh
+HARNESS_PERL_SWITCHES=-MDevel::Cover prove -lr
+cover
+```
+
+You need Devel::Cover installed.
+
+## What's here
+
+| 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. |
+
+## The require-and-stub pattern
+
+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.
+
+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 { ... }
+```
+
+### One-liner (sub definitions precede the main call)
+
+Used by most `bin/` CLI tools, which already define `sub main` and dispatch
+to it at the bottom:
+
+```perl
+#!/usr/bin/env perl
+use strict; use warnings;
+
+sub main {
+    ...
+    return 0;
+}
+exit main(\@ARGV) if !caller(0);
+
+sub helper { ... }
+```
+
+`!caller(0)` is true at script invocation (depth-0 frame absent) and false
+under `require`.
+
+## Sub-stubbing in tests
+
+`miniserv.t` is the canonical example. The pattern:
+
+1. `require` the script. The guard skips its main body.
+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.)
+   yourself instead of running the config loader.
+4. Call the sub under test. Assert on contract (status code, presence of
+   required headers, structural balance of emitted markup) — not on
+   cosmetics like exact wording or class names.
+
+Tying tests to the contract rather than the rendering lets the UI evolve
+without breaking the test, while still catching real regressions.
+
+## Tiered coverage policy
+
+Not all 268 modules deserve the same investment.
+
+- **Tier 1 — security-critical, network-facing.** `miniserv.pl`,
+  `web-lib-funcs.pl`, `acl/`, auth, file upload paths. Comprehensive
+  contract tests; mock filesystem and `backquote_command` for parser
+  coverage of external-binary output.
+- **Tier 2 — active refactor surface.** Currently `nftables/`, `firewalld/`,
+  and whichever module is being reviewed. Mandatory tests for new code;
+  `perlcritic.t` at severity 5 as a gate.
+- **Tier 3 — stable OS-config wrappers.** Covered by `compile.t` plus
+  optional per-module `perlcritic.t`. Don't chase line coverage on parsers
+  for config files that haven't changed in a decade.
+
+The goal is not coverage-as-a-number. It's:
+
+- Every parser round-trips its serializer.
+- Every privilege boundary has a test.
+- Every external-binary call has a mock-driven test for its output parser.
+
+## Adding a per-module test directory
+
+```
+yourmodule/
+  t/
+    perlcritic.t   # see nftables/t/perlcritic.t for the template
+    run-tests.t    # see nftables/t/run-tests.t for the WEBMIN_CONFIG / tmpdir setup
+```
+
+A module's tests are reachable from `prove -lr` at the repo root (no
+path arg, so the recursive walk starts at the cwd). `prove -lr t` only
+walks within `t/` and will miss `<module>/t/`.
+
+## Caveats
+
+- `WEBMIN_COMPILE_T_STRICT=1` turns missing-CPAN-module skips into failures.
+  Use this in CI on a fully-provisioned image; leave it off on dev boxes
+  where optional deps (`Pod::Simple::Wiki`, `Encode::Detect::Detector`) may
+  not be installed.
+- `.pl` is also the Polish translation suffix in Webmin. `compile.t` skips
+  `<file>.pl` when a sibling `<file>` (no extension) exists; this catches
+  `config.info.pl` and `module.info.pl` data files without a hardcoded list.
+

t/compile.t

@@ -0,0 +1,90 @@
+#!/usr/local/bin/perl
+# Verify every .pl and .cgi in the tree parses (perl -c).
+#
+# Catches syntax and `use` breakage from bulk refactors without having
+# to load every page in a browser. The test is the first line of defence
+# for the "we changed thousands of files mechanically, did anything
+# break" question.
+#
+# Skipped:
+#   - $file.pl when a sibling $file (no .pl) exists. Webmin uses .pl as
+#     the Polish translation suffix, so config.info.pl, module.info.pl,
+#     etc. are data files, not Perl.
+#   - Files that fail only because of a missing CPAN module. The file
+#     itself parses, but `use Foo::Bar` can't resolve at compile time.
+#     Treated as a skip so missing optional deps don't gate the suite.
+#     Set WEBMIN_COMPILE_T_STRICT=1 to turn these into failures.
+#
+# Speed: ~12 seconds for the full tree (~3.4k files). Narrow with
+# WEBMIN_COMPILE_T_FILTER=<regex> when iterating on one module.
+
+use strict;
+use warnings;
+use Test::More;
+use File::Find;
+use File::Basename qw(dirname);
+use File::Spec;
+use Cwd qw(abs_path);
+
+my $root = abs_path(File::Spec->catdir(dirname(__FILE__), '..'));
+chdir($root) or die "chdir($root): $!";
+
+my $filter = $ENV{WEBMIN_COMPILE_T_FILTER};
+my $strict = $ENV{WEBMIN_COMPILE_T_STRICT};
+
+my @files;
+find({
+	no_chdir => 1,
+	wanted => sub {
+		return if -d;
+		# .pl and .cgi files, plus extensionless files in bin/ with a
+		# perl shebang. The shebang check keeps us from compile-checking
+		# arbitrary non-perl files just because they share a directory.
+		my $name = $File::Find::name;
+		my $is_pl_or_cgi = $name =~ /\.(pl|cgi)\z/;
+		my $is_bin_dotless = $name =~ m{^\./bin/([^/]+)\z} && $1 !~ /\./;
+		return unless $is_pl_or_cgi || $is_bin_dotless;
+		# Skip the Polish translations that share the .pl suffix.
+		if ($is_pl_or_cgi && $name =~ m{(.+)\.pl\z}) {
+			my $base = $1;
+			return if -f "$base";
+			}
+		# For extensionless bin/ scripts, require a perl shebang.
+		if ($is_bin_dotless) {
+			open(my $fh, '<', $name) or return;
+			my $shebang = <$fh>;
+			close($fh);
+			return unless defined $shebang && $shebang =~ /^#!.*\bperl\b/;
+			}
+		push(@files, $name);
+		},
+	}, '.');
+
+@files = sort @files;
+@files or BAIL_OUT("found no .pl/.cgi/bin scripts under $root");
+
+if ($filter) {
+	@files = grep { /$filter/ } @files;
+	@files or do { diag("filter '$filter' matched zero files"); plan skip_all => "no files match filter"; };
+	}
+
+diag("compile-checking " . scalar(@files) . " files");
+
+for my $f (@files) {
+	my $rel = $f;
+	$rel =~ s{^\./}{};
+	my $out = qx{perl -I. -c -- "$rel" 2>&1};
+	if ($out =~ /\bsyntax OK\b/) {
+		pass("$rel compiles");
+		}
+	elsif (!$strict && $out =~ /Can't locate (\S+\.pm) in \@INC/) {
+		SKIP: { skip("$rel: missing optional CPAN module $1", 1); }
+		}
+	else {
+		fail("$rel compiles");
+		diag($out);
+		}
+	}
+
+done_testing();
+