Add unit testing harness by dtinth · Pull Request #3828 · jamulussoftware/jamulus

dtinth · 2026-07-23T14:55:48Z

Short description of changes

Added a unit test harness (based on QtTest) with a first test suite for the protocol layer, along with a CI workflow covering:

Linux (Qt 5.15)
Linux (Qt 6)
Linux with ASan/UBSan
Linux with coverage
macOS
Windows

No production code is changed.

The suite (19 cases) covers:

Golden frames: fixed hex literals of what CProtocol emits on the wire.
Frame acceptance/rejection: a valid frame is accepted while invalid ones rejected with following cases tests:
- truncated
- bad-CRC
- wrong-length
- junk frames
Round-tripping: Two connected CProtocol instances, observed via an event log of received signals.
Regression test for Check the size of PROTMESSID_ACKN messages #302 (ACKN with valid checksum but empty body caused an out-of-bounds read, found by fuzzing in 2020). The ASan job is what gives the "no OOB" part teeth.

CHANGELOG: SKIP

Context: Fixes an issue?

First step toward #2428 (automated testing in CI). This PR only covers unit-testing. A real client/server smoke test is planned separately.

Does this change need documentation? What needs to be documented and how?

No.

Status of this Pull Request

Working implementation, CI-verified.

What is missing until this pull request can be merged?

Nothing known.

Notes for reviewers:

Running locally:

mkdir build-test && cd build-test
qmake ../src/test/test.pro
make check

Alternatively:

python3 .github/scripts/run-unit-tests.py build
python3 .github/scripts/run-unit-tests.py run

The test binary's exit code is ignored because on Windows runners the binary's stdout/exit behavior is unreliable. We have a Python script inspect the generated JUnit XML instead (details in run-unit-tests.py).
We built a bespoke test results reporting script in Python. We considered richer PR annotations via a report action (e.g. dorny/test-reporter), but they require enabling extra permissions that PRs from forks do not have (like checks: write).
The workflow is admittedly long (~220 lines); most of it is the per-platform Qt/MSVC setup. We did not refactor them into reusable actions to keep this PR self-contained.
I built this with help from my coding agent (Claude Code with Fable 5). You can read the full conversation here (but it's quite long!). It took a lot of steering to arrive at the current shape.

Checklist

I've verified that this Pull Request follows the general code principles
I tested my code and it does what I want
My code follows the style guide
I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
I've filled all the content above

Introduces src/test/, a QtTest-based unit test target (jamulus-test) covering CProtocol's on-wire framing contract: - golden-frame tests pin the exact bytes production emits today for a fixed-length and a variable-length message body, so any accidental wire format change fails loudly instead of silently -- the expected hex string is built field by field (TAG, message ID, sequence counter, data length, data, CRC), each `+=` line commented with what that field is, against a fresh CProtocolTester's SentFrames() - a frame acceptance/rejection contract test trio (AcceptValidFrame, RejectInvalidFrame with bad CRC/length/truncation/junk rows, and IgnoreAcknWithEmptyBody -- the regression test for jamulussoftware#302, fixed in 024ebb4: an ACKN message with a valid checksum but no data caused an out-of-bounds read; the crafted frame is still well-formed so it's accepted at the frame level, but must be silently dropped with no signal fired, and the ASan/UBSan matrix job is what gives the "no OOB" part of that its teeth) checks how many frames the receiving side actually parsed and accepted, building each frame imperatively from a real sent frame (LastSentFrame()) plus a small set of named mutation helpers - one round-trip test through a connected sender/receiver pair (CProtocolTester) sends a message on one side and asserts, against an event log of everything the other side received, that exactly the expected signal fired with the expected arguments CProtocolTester (src/test/protocoltester.h) is the one public type this header exposes: a struct-like pair of CProtocol instances (Sender, Receiver) wired together in both directions -- the same wiring CChannel uses for two peers, including routing acknowledgements back so the sender's queue advances -- plus: - a sent frame log: every frame Sender hands to MessReadyForSending is recorded as both a hex string (SentFrames(), for golden frame comparisons) and raw bytes (LastSentFrame(), for tests that mutate a real frame); a fresh instance's first send has sequence counter 0, which is what makes golden frames byte-for-byte reproducible - a receiver-side acceptance count: ReceivedAndAcceptedMessageCount() counts frames that passed frame parsing on the receiver side and were handed to ParseMessageBody(); a failed parse -- whether from a real send or a malformed frame injected via SendRawBytes() -- is a silently dropped, countable non-event rather than an assertion failure, and is tracked per direction so an ACK flowing back to the sender never affects the receiver's own count - a received signal log: every non-CLM "receiving" signal CProtocol emits from ParseMessageBody (protocol.h's ChangeJittBufSize..RecorderStateReceived block, 21 signals) is wired to append one formatted "SignalName(arg1, arg2)" line to ReceivedLog(), so a test can QCOMPARE the whole log against what it expects -- which also proves, for free, that no other wired signal fired - ToByteArray()/FromByteArray(), the frame mutators TruncateBy()/CorruptCRC()/SetDeclaredLength(), and ReplaceIdAndBody() (rebuilds a frame with a different ID/body, recomputing length and CRC) for crafting ID/body combinations a real Create*Mes() call can't produce Tests hold their own frames/expectations and call these helpers directly, with no builder chain, lambda-taking capture function, or shared error-message plumbing in the public surface to look through. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HGdLtGxMrbExBFG3aReNAH

.github/scripts/run-unit-tests.py is a single Python (stdlib only) driver for building and running the unit test suite across all three CI platforms (Linux, macOS, Windows) with one "build" and one "run" subcommand. On Windows it also bootstraps the MSVC environment itself, using the same env-diffing technique windows/deploy_windows.ps1's Initialize-Build-Environment already uses (call vcvarsall.bat, diff the environment before/after) -- it has to happen again here rather than once in the workflow, since build and run are two separate GitHub Actions steps and an environment set up in one does not survive into the next. "run" is also the test-count gate: after running the binary it parses test-results.xml (stdlib only) and exits nonzero unless the suite reports at least one test and zero failures/errors -- the binary's own exit code is ignored throughout, since it's unreliable on Windows runners, so test-results.xml is the sole source of truth. .github/scripts/summarize-test-results.py parses that same JUnit XML and appends a pass/fail table (plus any failing test names/messages) to the job summary. It is reporting only and always exits 0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HGdLtGxMrbExBFG3aReNAH

.github/workflows/unit-tests.yml runs the new suite on every push and pull request touching src/**, this workflow file, or the scripts it calls (plus workflow_dispatch for manual runs), mirroring the trigger shape of the repository's other main-branch check workflows. A 6-job matrix covers: - Linux (Qt 5.15, ubuntu-22.04) -- proves the suite's Qt >= 5.12 floor - Linux (Qt 6, ubuntu-24.04) - macOS (Qt 6) and Windows (Qt 6, MSVC) -- Qt is set up by reusing autobuild.yml's own setup scripts (.github/autobuild/mac.sh, windows.ps1) and its exact cache key/paths, so this job shares that cache instead of keeping a separate one; heavier than this suite strictly needs (extra Qt modules, a 32-bit Qt, jom on Windows) in exchange for that shared cache - Linux (Qt 6, ASan/UBSan) -- same toolchain with sanitizers enabled via qmake command line flags, to catch memory/UB issues a plain build wouldn't - Linux (Qt 6, coverage) -- same toolchain instrumented with gcov, producing an HTML report artifact plus a Markdown summary table for src/ (excluding the test suite itself) Every job builds and runs the suite via run-unit-tests.py, which is also the gate; Summarize test results (always run) and the coverage steps (always run, coverage jobs only) still render their output even if that gate failed. The workflow only needs `permissions: contents: read` throughout. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HGdLtGxMrbExBFG3aReNAH

Copilot

Pull request overview

Adds an initial QtTest-based unit test harness for Jamulus’ protocol layer, plus CI automation to build/run the suite across multiple platforms/configurations (including sanitizers and coverage), without changing production runtime code.

Changes:

Introduces a protocol test suite with golden on-wire frame checks, malformed-frame rejection cases, round-trip signal logging, and a regression test for the ACKN empty-body OOB issue (#302).
Adds a dedicated qmake project (src/test/test.pro) and helper harness (CProtocolTester) for wiring two CProtocol instances and logging frames/signals.
Adds a GitHub Actions workflow to build/run tests on Linux/macOS/Windows and publish JUnit + optional coverage artifacts, with summary reporting.

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
`src/test/tst_protocol.cpp`	New protocol-focused QtTest suite (golden frames, invalid-frame rejection, round-trip, #302 regression).
`src/test/test.pro`	qmake project to build the unit test binary in a headless/server-only configuration.
`src/test/protocoltester.h`	Test harness wiring `CProtocol` instances together, with frame mutation helpers and signal logging.
`.gitignore`	Ignores build/test output artifacts (`build-test/`, JUnit/XML and text output).
`.github/workflows/unit-tests.yml`	New CI workflow to build/run the unit tests across platforms, with sanitizer/coverage variants and artifact uploads.
`.github/scripts/summarize-test-results.py`	Generates a per-job summary table (and failing tests list) from the produced JUnit XML.
`.github/scripts/run-unit-tests.py`	Cross-platform build/run driver that gates success based on JUnit XML contents (not process exit code).

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

Both from PR review on jamulussoftware#3828: - src/test/protocoltester.h: CProtocolTester::TruncateBy() now clamps the resize at 0, so truncating by more than the frame's size yields an empty frame instead of a negative resize (which wraps to a huge size_t and would abort/corrupt memory). - .github/scripts/run-unit-tests.py: msvc_environment()'s snapshot() helper now checks the subprocess return code and fails fast with the command plus its captured stdout/stderr if it's nonzero, instead of letting a broken vcvarsall.bat call surface later as a confusing compiler-not-found error. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HGdLtGxMrbExBFG3aReNAH

Copilot

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.

The header uses std::max but relied on transitive inclusion via protocol.h -> util.h. Include it directly so the header stays self-contained (same practice as util.h), per PR review. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HGdLtGxMrbExBFG3aReNAH

Copilot

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.

The key hardcoded the version string alongside the env.QT_VERSION definition above, so the two could drift after a Qt update. The interpolated key is byte-identical today, keeping the cache shared with autobuild.yml's macOS job. Per PR review. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HGdLtGxMrbExBFG3aReNAH

Copilot

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.

ann0see · 2026-07-23T16:02:07Z

+ * Copyright (c) 2026
+ *
+ * Author(s):
+ *  The Jamulus Development Team


Probably add your name first

Replaced with "dtinth" following the precedent in rpcserver.cpp. Addressed in ccbb7f5.

ann0see · 2026-07-23T16:02:21Z

+ * Author(s):
+ *  The Jamulus Development Team
+ *
+ * Licensed under AGPL 3.0 or any later version. See COPYING for details.


@pljones Is this sufficient?

@ann0see Updated to full AGPL header in 6094fbe. However, as this is new code, the clause about existing code being GPL is dropped from this file.

ann0see · 2026-07-23T16:03:03Z

+    HEADLESS \
+    NO_JSON_RPC \
+    HAVE_STDINT_H \
+    QT_NO_DEPRECATED_WARNINGS


Why don't you show those?

@ann0see Thanks for catching this.

Dropping QT_NO_DEPRECATED_WARNINGS surfaced a deprecation warning the build output on util.cpp:1556 (QLibraryInfo::location() → path(), deprecated in Qt 6). My agent hid it as CI noise but I agree that the the test workflow should surface it rather than hide it. Addressed in ccbb7f5.

The Unix branch already appends qmake_extra_args(); the Windows branch ignored it, which was harmless (only the Linux ASan job sets the variable) but inconsistent, and would silently drop the args from any future Windows build variant. Per PR review. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HGdLtGxMrbExBFG3aReNAH

Copilot

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.

The other two gate messages already include it. Per PR review. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HGdLtGxMrbExBFG3aReNAH

Copilot

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.

The scripts had inherited the C++ sources' space-inside-parens style; the repo's existing Python (.github/autobuild/get_build_vars.py, tools/generate_json_rpc_docs.py) uses conventional PEP8 formatting, so follow that. Reformatted with black; no behavior change (AST-identical before and after). Per PR review. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HGdLtGxMrbExBFG3aReNAH

Copilot

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.

ann0see · 2026-07-23T18:23:34Z

GitHub doesn't show any individual test cases like gitlab - I suppose

ann0see · 2026-07-23T18:52:39Z

+env:
+  # Must match autobuild.yml's macOS/Windows QT_VERSION -- Qt setup below
+  # reuses those jobs' scripts and cache.
+  QT_VERSION: 6.10.2


Then we should try to find out how to only have one source of truth (the autobuild.yaml) to set this.

Normally I would extract the common setup step into a composite action which gets reused by both workflows. I think this is cleaning but result in a larger change.

My agent also recommended another alternative: Have unit-tests.yml parse autobuild.yml to extract QT_VERSION=. My knee-jerk reaction is that it sounds quite hacky to me, but it works, keeps a single source of truth, and doesn't require refactoring existing workflows.

Do you have a preference?

(Merging as-is or using the autobuild.yml approach, and then doing proper action refactoring as a follow-up PR is also an option.)

Not merging as is. I think for now we could parse autobuild.yaml but if you prefer the other cleaner approach it would be better.

Done in 3b4612a — took the parse approach for now (a bigger refactor queued for a more focused, follow-up PR).

ann0see · 2026-07-23T18:53:06Z

+  QT_VERSION: 6.10.2
+
+  # gcovr version used by the coverage job below, pinned for reproducible output.
+  GCOVR_VERSION: 8.6


Should then be bumped like our other dependencies automatically

gcovr version bump added in 383c143.

ann0see · 2026-07-23T18:54:17Z

+      - name: Install Qt (macOS, via autobuild's mac.sh)
+        if: runner.os == 'macOS'
+        env:
+          JAMULUS_BUILD_VERSION: 0.0.0 # required by the script's validation; unused by "setup"


Then the script should be updated such that it doesn't need this if we just run setup

JAMULUS_BUILD_VERSION validation is now optional in setup in ebb39ce.

dtinth · 2026-07-23T19:30:35Z

@ann0see GitHub doesn't have any built-in test reporting AFAIK, only job summaries.

gcovr's version was hand-pinned with no update mechanism, unlike the other pinned dependencies. Add a bump-dependencies.yml matrix entry following the existing pattern: gcovr publishes GitHub releases (like aqt/create-dmg/jack/etc.), so reuse the same gh-release-tag lookup and extend the perl regex scan to match the "GCOVR_VERSION: <ver>" YAML pin. Not marked Changelog-worthy since gcovr is CI-only tooling, never shipped to end users -- same treatment as aqt and choco-jom. Per PR review. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HGdLtGxMrbExBFG3aReNAH

…stage mac.sh and windows.ps1 validated JAMULUS_BUILD_VERSION unconditionally at the top of the script, even though only the stages that actually read it need it: mac.sh's "build" (via mac/deploy_mac.sh, which reads the inherited env var directly for the .dmg/.pkg names) and "get-artifacts", and windows.ps1's "get-artifacts" (deploy_windows.ps1 computes its own version from source and never touches the env var, so "build" doesn't need it there). Move the check into a validate_build_version()/Get-Validated-Build-Version() helper called from just those stages, so "setup" runs without the variable set. This lets unit-tests.yml drop its JAMULUS_BUILD_VERSION: 0.0.0 dummy env, added only to satisfy the old unconditional check for a stage that never used it. Per PR review. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HGdLtGxMrbExBFG3aReNAH

Copilot

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.

A negative iBytes used to enlarge the frame via Size() - iBytes, which contradicts the helper's purpose and could hide mistakes when crafting mutation inputs; it is a no-op now. Per PR review. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HGdLtGxMrbExBFG3aReNAH

Copilot

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated no new comments.

ann0see

Curious of other test cases then too obviously. But not part of this PR.

autobuild.yml's macOS main build entry becomes the single source of truth; a step extracts QT_VERSION from it (failing loudly if the parse does not yield exactly one plausible version) instead of pinning a copy here that could drift. Per PR review. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HGdLtGxMrbExBFG3aReNAH

Copilot

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated no new comments.

dtinth-claw Bot and others added 3 commits July 23, 2026 20:42

Copilot AI review requested due to automatic review settings July 23, 2026 14:55

Copilot started reviewing on behalf of dtinth July 23, 2026 14:56 View session

dtinth mentioned this pull request Jul 23, 2026

Testing harness dtinth/jamulus#3

Open

dtinth-claw Bot mentioned this pull request Jul 23, 2026

Scope the first upstream PR dtinth/jamulus#7

Closed

Copilot AI reviewed Jul 23, 2026

View reviewed changes

Comment thread src/test/protocoltester.h

Comment thread .github/scripts/run-unit-tests.py Outdated

Copilot AI review requested due to automatic review settings July 23, 2026 15:17

Copilot started reviewing on behalf of dtinth July 23, 2026 15:17 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

Comment thread src/test/protocoltester.h

Copilot AI review requested due to automatic review settings July 23, 2026 15:34

Copilot started reviewing on behalf of dtinth July 23, 2026 15:35 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

Comment thread .github/workflows/unit-tests.yml Outdated

Copilot AI review requested due to automatic review settings July 23, 2026 15:52

Copilot started reviewing on behalf of dtinth July 23, 2026 15:53 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

Comment thread .github/scripts/run-unit-tests.py

ann0see added this to the Release 4.0.0 milestone Jul 23, 2026

ann0see added release process Changes to the release process AI AI generated or potentially AI generated labels Jul 23, 2026

ann0see assigned ann0see and dtinth Jul 23, 2026

ann0see added this to Tracking Jul 23, 2026

github-project-automation Bot moved this to Triage in Tracking Jul 23, 2026

ann0see moved this from Triage to Waiting on Team in Tracking Jul 23, 2026

ann0see requested changes Jul 23, 2026

View reviewed changes

github-project-automation Bot moved this from Waiting on Team to Waiting externally in Tracking Jul 23, 2026

Copilot AI review requested due to automatic review settings July 23, 2026 17:00

Copilot started reviewing on behalf of dtinth July 23, 2026 17:00 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

Comment thread .github/scripts/run-unit-tests.py Outdated

Name the results file in the failed-tests gate message

9a26abc

The other two gate messages already include it. Per PR review. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HGdLtGxMrbExBFG3aReNAH

Copilot AI review requested due to automatic review settings July 23, 2026 17:14

Copilot started reviewing on behalf of dtinth July 23, 2026 17:15 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

Comment thread .github/scripts/run-unit-tests.py

Comment thread .github/scripts/summarize-test-results.py Outdated

Copilot AI review requested due to automatic review settings July 23, 2026 17:24

Copilot started reviewing on behalf of dtinth July 23, 2026 17:24 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

ann0see requested a review from pljones July 23, 2026 18:24

ann0see reviewed Jul 23, 2026

View reviewed changes

dtinth-claw Bot and others added 2 commits July 24, 2026 02:45

Copilot AI review requested due to automatic review settings July 23, 2026 19:50

Copilot started reviewing on behalf of dtinth July 23, 2026 19:51 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

Comment thread src/test/protocoltester.h Outdated

Copilot AI review requested due to automatic review settings July 23, 2026 20:05

Copilot started reviewing on behalf of dtinth July 23, 2026 20:05 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

ann0see reviewed Jul 24, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 24, 2026 08:43

Copilot started reviewing on behalf of dtinth July 24, 2026 08:43 View session

Copilot AI reviewed Jul 24, 2026

View reviewed changes

Uh oh!

Conversation

dtinth commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

ann0see commented Jul 23, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dtinth commented Jul 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

dtinth commented Jul 23, 2026 •

edited

Loading