Conversation
Adds the project's first automated tests. Until now the 13 scripts in tests/ were wired into no runner and no workflow triggered on pull requests. - lint: bash -n plus shellcheck at error severity - unit-tests: the existing suite on Debian 12, Ubuntu 22.04/24.04, Alpine 3.20 and Fedora 41 - systemd-integration / openrc-integration: exercise setup_autostart and main_service_remove against real init systems Alpine installs only bash; adding coreutils would shadow busybox and hide the differences that row exists to catch.
A quarantine list rots in a specific way: the upstream fix lands, the entry stays, and the suite keeps advertising a failure it no longer has. Nothing in the output said so, because a quarantined test that passes was reported as an ordinary PASS. run-all.sh now counts those and prints "! still listed as quarantined, but passing — the entry can be removed", with a matching line in the summary. Leaving an entry in place stays harmless, so the list is correct in any merge order; it is simply visible now instead of silent. Also names, in the workflow, which PR retires each Alpine entry (SamNet-dev#145, SamNet-dev#146, SamNet-dev#148), so the maintainer can remove them as those merge rather than having to rediscover the mapping.
v4 targets Node.js 20, which was removed from GitHub-hosted runners on 2026-09-16. Jobs
using it are now forced onto Node.js 24 and emit a deprecation annotation on every job:
Node.js 20 is deprecated. The following actions target Node.js 20 but are being
forced to run on Node.js 24: actions/checkout@v4
v6 is the current major and targets Node.js 24. It is a drop-in for plain checkout usage;
the one behavioural change (persist-credentials moving to $RUNNER_TEMP) only affects
Docker container actions and needs runner >= 2.329.0, which hosted runners satisfy.
build-engine.yml needs nothing: it uses only docker/* actions and has no checkout step.
ci: drop the inline comments on the actions/checkout bump
Repeating the rationale above each of the four uses: lines is noise; the version
is self-explanatory and the reasoning belongs in the commit that made the change.
rvalitov
force-pushed
the
ci/tests-matrix
branch
from
September 17, 2026 20:03
7682c4b to
c2ab72e
Compare
rvalitov
marked this pull request as ready for review
September 18, 2026 08:12
Contributor
Author
|
I marked this as ready for review, because this PR actually works but skips some tests (quarantine) that fail because other PRs are not merged yet. So you can either:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The repository has 13 test scripts in
tests/and no runner, no aggregator, and no workflow that triggers on a pull request. The only workflow,build-engine.yml, isworkflow_dispatch-only and builds the engine image. Meanwhile the README lists Ubuntu, Debian, CentOS, RHEL, Fedora, Rocky, AlmaLinux and Alpine as supported, and the script ships two init-system paths — none of which is verified on any commit.This adds the project's first automated test pipeline. No product code is modified.
What runs
lintbash -non every script, plusshellcheck -S errorunit-testssystemd-integrationsetup_autostart/main_service_removeagainst real systemdopenrc-integrationTriggers on
pull_request,pushtomain, andworkflow_dispatch. The workflow requestscontents: readonly and uses no secrets — it executes fork code, so it usespull_requestand neverpull_request_target.The distro matrix
debian:12,ubuntu:22.04,ubuntu:24.04,alpine:3.20,fedora:41.Alpine is the row that earns its keep: its busybox userland differs from GNU in
sed,grep,mktemp,date,flockandnetstat, and it is a documented supported platform that has never been exercised. It has already surfaced real defects — see the quarantine note below.fail-fast: false, so one red distro still reports all the others.Design decisions a reviewer may question
docker run, not the job-levelcontainer:key. JavaScript actions — includingactions/checkout— execute with anodebinary inside the job container, and the runner does not inject one.debian:12andalpine:3.20ship neithernodenorgit, so acontainer:job fails before any step that could install them.Alpine installs only
bash. Addingcoreutilsordiffutilswould shadow busybox and hide exactly the differences that row exists to detect. The suite is expected to meet busybox on its own terms.Fedora needs
diffutils. It ships bash but nodiff, and one test in the suite usesdiff— which, with2>/dev/null, made a missing binary look like a content mismatch.The systemd job runs natively on
ubuntu-24.04. That image is a full VM with systemd as PID 1 and Docker already installed, so the generated unit'sRequires=docker.serviceresolves against a real unit. Running it in a privileged container instead would need--privileged --cgroupns=hostand would be a less faithful substitute.Containers need shims that a real host gets for free. OpenRC refuses to run any service without
/run/openrc/softleveland will not start a service whose hard dependencies are unresolved, so the OpenRC test creates the marker and adockerstub. Those are in the test harness, not the workflow.The quarantine mechanism, and why it is not an excuse
Three tests fail on this repository today. Shipping a pipeline that is red on arrival trains everyone to ignore it, so
tests/run-all.shsupports a per-image quarantine list:QUARANTINE, counted separately, and never counted as passingCurrent entries, each with the fix that retires it:
test_client_mss.shtest_traffic_reset.shflockhas no-w, so the lock call fails whilecommand -v flocksucceeds, and the guard never firestest_guest.shdate -d "+24 hours"is invalid on busybox, and thedate -r <epoch>fallback fails tooLeaving an entry in place after its fix lands is harmless — a quarantined test that passes is reported as
PASS— so this list is correct in any merge order.run-all.shflags such entries as! still listed as quarantined, but passing, so a stale entry is visible rather than silently ignored.Evidence
The full matrix has been run locally against this branch, reproducing the workflow's exact per-image install command:
The integration jobs were verified against real init systems: 14/14 on a container running systemd as PID 1, and 16/16 under OpenRC on Alpine.
Both integration scripts are mutation-tested — deliberately breaking
ExecStartor the OpenRCstart()makes them fail with a readable diagnostic, so they are known to have signal rather than merely being green.Known interactions
New test files are picked up automatically.
run-all.shglobstests/test_*.sh, so any PR that adds a test file starts running it across the matrix with no workflow change.#143 adds ten test files, seven of which have not been exercised on Alpine or Fedora. If one fails there, this pipeline will report it — which is the point of the pipeline, but it means a red run immediately after #143 merges may be #143's tests rather than a defect in this PR.
First-time contributor gating. GitHub withholds workflow runs from new contributors until a maintainer approves them, so the first run after this merges may show no checks at all. That is expected, not a failure.
What is not covered
There is no end-to-end test that installs the proxy and pushes traffic through it. The installer is interactive with no non-interactive flag — the script's only
[ ! -t 0 ]fallback is for secret removal, not installation — so driving it in CI would need a product change. That is deliberately out of scope here.The integration tests therefore target
setup_autostart()andmain_service_remove()directly, which is where the init-system divergence actually lives.