From ab37804ef99ce1b9af921f15cfdda858f4034cc9 Mon Sep 17 00:00:00 2001 From: Joe Cooper Date: Sun, 17 May 2026 23:17:15 -0500 Subject: [PATCH 1/2] Add docs for coverage --- t/README.md | 147 ++++++++++++++++++++++++++++++++++++++++++++++++++++ t/compile.t | 90 ++++++++++++++++++++++++++++++++ 2 files changed, 237 insertions(+) create mode 100644 t/README.md create mode 100644 t/compile.t diff --git a/t/README.md b/t/README.md new file mode 100644 index 000000000..1ced97dc6 --- /dev/null +++ b/t/README.md @@ -0,0 +1,147 @@ +# 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 t` at the root via the +`-r` recursive walk. + +## 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 + `.pl` when a sibling `` (no extension) exists; this catches + `config.info.pl` and `module.info.pl` data files without a hardcoded list. + diff --git a/t/compile.t b/t/compile.t new file mode 100644 index 000000000..214055037 --- /dev/null +++ b/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= 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(); + From d81eb13f22bba5ff2bfce7f8080b777ecead2a48 Mon Sep 17 00:00:00 2001 From: Joe Cooper Date: Sun, 17 May 2026 23:33:59 -0500 Subject: [PATCH 2/2] Run tests on PR, add docs --- .github/workflows/tests.yml | 19 +++++++++++++++++++ t/README.md | 5 +++-- 2 files changed, 22 insertions(+), 2 deletions(-) create mode 100644 .github/workflows/tests.yml diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml new file mode 100644 index 000000000..a07d9ff89 --- /dev/null +++ b/.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 diff --git a/t/README.md b/t/README.md index 1ced97dc6..27f704ae7 100644 --- a/t/README.md +++ b/t/README.md @@ -132,8 +132,9 @@ yourmodule/ run-tests.t # see nftables/t/run-tests.t for the WEBMIN_CONFIG / tmpdir setup ``` -A module's tests are reachable from `prove -lr t` at the root via the -`-r` recursive walk. +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 `/t/`. ## Caveats