Prep for testing, add compile.t

[swelljoe]

Same prep for testing as this PR does for Webmin.

This one is hairier, as we have execute_webmin_function in json-lib.pl which does this:

	eval "
		\%pkg::ENV = \%ENV;
		package $pkg;
		do \$cmd;
		die \$@ if (\$@);
		";
	exit($@ ? 1 : 0);

To run scripts, which sets caller and also expects side effects. I can't figure out why it does it this way, but it's easy/safe to work around with an environment variable VIRTUALMIN_NO_MAIN that gets set in tests for the handful of effected files, which is checked instead of caller.

Also adds a compile.t test which just does perl -c across all Perl files, and a t/README.md documenting how to make tests and the special case we're doing for execute_webmin_command.

[Copilot]

Pull request overview

Prepares this codebase for unit testing by guarding script "main bodies" so they don't execute when the file is required as a library, and adds a compile.t smoke test that runs perl -c over every .pl/.cgi in the tree. Mirrors the equivalent Webmin PR, but introduces a new VIRTUALMIN_NO_MAIN env-var guard for .pl files because execute_webmin_script in json-lib.pl runs scripts via do $cmd inside an eval, which makes caller truthy and would defeat the simpler unless (caller) form used for .cgi files.

Changes:

• Wrap script main bodies in unless ($ENV{VIRTUALMIN_NO_MAIN}) { ... } (CLI .pl) or unless (caller) { ... } (.cgi) for ~13 scripts.
• Hoist file-scope lexicals ($etcdir, etc.) above the guard in restore-config-revision.pl and list-config-revisions.pl so subs after the guard still see them.
• Add t/compile.t (per-file perl -c test with optional skip on missing CPAN modules) and t/README.md documenting the test pattern, guard rationale, and tiered coverage policy.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
t/README.md	New documentation for the test suite, the require-and-stub pattern, why two guards exist, and the coverage policy.
t/compile.t	New perl -c test across all .pl/.cgi, with filter and strict-mode env vars, and CPAN-skip handling.
backup.pl	Wrap main body in VIRTUALMIN_NO_MAIN guard to expose the print/cb subs to tests.
bwgraph.cgi	Wrap main body in unless (caller) to expose usage_colours/usage_for_days etc.
check-scripts.pl	Add VIRTUALMIN_NO_MAIN guard around main body.
downgrade-licence.pl	Add VIRTUALMIN_NO_MAIN guard around main body.
functional-test.pl	Add VIRTUALMIN_NO_MAIN guard around main body so run_test/conversion subs are testable.
info.pl	Add VIRTUALMIN_NO_MAIN guard around main body.
link.cgi	Wrap main body in unless (caller) so preview/proxy helpers can be required by tests.
lookup-domain-daemon.pl	Hoist use POSIX/Socket; above the guard and wrap main body.
quotas.pl	Add VIRTUALMIN_NO_MAIN guard around main body.
restore-config-revision.pl	Hoist file-scope lexicals ($etcdir, $target_dir, $dry_run, $depth, $git_repo) above the guard, then wrap main body.
list-config-revisions.pl	Same pattern as restore-config-revision.pl: hoist $etcdir/$depth/$git_repo, wrap main body.
spamtrap.pl	Add VIRTUALMIN_NO_MAIN guard around main body.
upload-api-docs.pl	Add VIRTUALMIN_NO_MAIN guard around main body.

Comments suppressed due to low confidence (2)

t/compile.t:64

• The filename is interpolated into a double-quoted shell string, so any path containing ", $, backticks, or backslashes would either break the command or, worse, allow shell metacharacter execution. While this repo's tree currently has no such filenames, it would be safer to invoke perl using a list-form system/IPC::Open3 (e.g. capturing via IPC::Run3 or open3) rather than qx{...} with shell interpolation, since compile.t is intended to be run by anyone who clones the tree (which could be in a path containing spaces or shell metacharacters — note that $rel is relative, but the perl invocation still inherits the cwd). At minimum, consider using single-quoted shell quoting and rejecting filenames containing '.

	my $out = qx{perl -I. -c -- "$rel" 2>&1};

t/compile.t:70

• The "missing optional CPAN module" detection only matches the first Can't locate ... in @INC line. If a file has both a missing optional module and a real syntax error, the test will be silently skipped and the syntax error hidden. Consider also requiring that no syntax error at / BEGIN failed lines unrelated to the missing module appear in $out before treating it as a skip, or at least emitting a diag of the full $out when skipping so a reviewer can sanity-check.

	elsif (!$strict && $out =~ /Can't locate (\S+\.pm) in \@INC/) {
		SKIP: { skip("$rel: missing optional CPAN module $1", 1); }
		}

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[iliaross]

Hey Joe!

I’m sorry, but I’m not comfortable with the direction of this PR, as well as webmin/webmin#2701 and webmin/webmin#2695, as-is.

I generally support adding tests, especially broad compile smoke tests, but changing production script flow primarily so files can be loaded by tests feels like the wrong order of operations. The production-code changes seem to be preparing for future tests rather than supporting tests added here.

I’d prefer we first improve or extract the code into cleaner, testable blocks, then add tests against those blocks.

On top of that, my concern is that these small guard tweaks can subtly change behavior or scope in old Perl code and give us false confidence. A compile test can pass while runtime behavior changes, especially around file-scope lexicals, package globals, and scripts loaded via do or require.

The compile test itself is valuable, but I think it should be separated from production refactors unless those refactors are independently justified and covered by tests that exercise the changed behavior.

For now, I’d strongly prefer we put this direction on hold until we do a broader cleanup of the code to fit proper modern Perl standards. I can do a proper broader cleanup after I finish working on Cloudmin 10, sometime this summer.

[swelljoe]

You can't safely do a large refactor without a test suite.

[swelljoe]

Fine, I'll change them all to use the variable method.

You cannot argue the variable method can have unintended effects. It doesn't rely on caller, at all.

I'm really not comfortable with embarking on any kind of major changes without a really good test suite. You act like you're afraid of breaking things, and then you want to do the thing that's guaranteed to break a lot of things before we have a good way to check for breakage. That doesn't make sense.

[jcameron]

Is this PR ready for review? Sorry I missed it initially ..