Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 8 additions & 11 deletions .github/copilot-instructions.md
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ firmware/
│ └── native/ # Linux/Portduino variants
├── protobufs/ # Protocol buffer definitions
├── boards/ # Custom PlatformIO board definitions
├── test/ # Native unit-test suites (count: test/native-suite-count)
├── test/ # Native unit-test suites (count = the test_* dirs, detected on the fly)
└── bin/ # Build and utility scripts
```

Expand Down Expand Up @@ -663,7 +663,7 @@ Most workflows can be triggered manually via `workflow_dispatch` for testing.

### Native unit tests (C++)

Unit tests in `test/` directory. The canonical suite count is in `test/native-suite-count`, cross-checked against `test/test_*` on every full run and by the `suite-count-check` CI job. **Never state the count as a literal anywhere else** - point at that file. The list below is a partial description of what suites cover, not an inventory:
Unit tests in `test/` directory. The canonical suite count is detected on the fly: the `test_*` directories under `test/` are the register, and `bin/run-tests.sh` cross-checks the suites that actually ran against them on every full run. **Never state the count as a literal anywhere** - it is whatever `test/test_*` contains right now. In CI, the `suite-shrinkage-check` job (`test_native.yml`) fails a PR that loses a `test_*` directory relative to its merge base unless the suite is named in the PR title, body, or a commit message - deleting a suite therefore requires saying so. The list below is a partial description of what suites cover, not an inventory:

- `test_admin_radio/` - LoRa region/config validation, AdminModule dispatch, node-DB metadata saves
- `test_fscommon_getfiles/` - bounded file-manifest walk (cap, depth, truncation reporting)
Expand Down Expand Up @@ -693,7 +693,7 @@ Unit tests in `test/` directory. The canonical suite count is in `test/native-su
- `test_utf8/` - UTF-8 utilities
- `test_warm_store/` - Warm-tier node store

**Preferred run command - `bin/run-tests.sh`** (defaults to the `coverage` env; emits a machine-readable verdict on the final line; update `test/native-suite-count` when adding or removing suites):
**Preferred run command - `bin/run-tests.sh`** (defaults to the `coverage` env; emits a machine-readable verdict on the final line; new `test_*` directories are picked up automatically):

```bash
./bin/run-tests.sh # all suites
Expand All @@ -716,29 +716,26 @@ Exit codes and verdicts (exact counts will vary; examples below are illustrative
| ---- | ---------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
| 0 | `GREEN` | All canonical suites ran, all passed, no ignored test cases |
| 1 | `RED` | At least one failure, build error, or sanitizer fault |
| 2 | `AMBER` | All that ran passed, but something was lost or unexplained: a suite silently went missing on a full run, individual test cases were skipped (`TEST_IGNORE`), `test/native-suite-count` disagrees with the `test/` directory count, or a suite left behind shared state it does not declare |
| 2 | `AMBER` | All that ran passed, but something was lost or unexplained: a suite silently went missing on a full run, individual test cases were skipped (`TEST_IGNORE`), or a suite left behind shared state it does not declare |
| 3 | `FILTERED` | A `-f` run completed cleanly; suites outside the filter were intentionally not run |

Examples - exact counts will vary by suite count and env:

```text
# GREEN: all suites ran and passed
RESULT: GREEN N/N suites passed [canonical: N/N]
RESULT: GREEN N/N suites passed, all CLEAN

# RED: real test failure
RESULT: RED 1 failed

# RED: sanitizer exit-time abort (all tests passed but process aborted at exit)
RESULT: RED exit-time abort (tests passed; likely sanitizer - see hint above)

# AMBER: native-suite-count disagrees with test/ directory count (too low)
RESULT: AMBER test/ has 24 suite directories but native-suite-count says 5 - update test/native-suite-count after registering new suites

# AMBER: native-suite-count disagrees with test/ directory count (too high)
RESULT: AMBER test/ has 24 suite directories but native-suite-count says 99 - update test/native-suite-count after removing suites
# AMBER: a suite silently went missing on a full run
RESULT: AMBER 23/24 suites ran (missing: test_radio) - all that ran passed

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Remove literal suite counts from the examples.

Line 735 contains 23/24, and Line 738 contains 1/24. Replace both values with symbolic placeholders such as N/M, as already used in Line 726.

As per coding guidelines, never state the number of test suites as a literal; discover it from the current test/test_* directories.

Suggested documentation fix
-RESULT: AMBER 23/24 suites ran (missing: test_radio) - all that ran passed
+RESULT: AMBER N/M suites ran (missing: test_radio) - all that ran passed
-RESULT: FILTERED 1/24 suites ran (not run: test_admin_radio test_atak …) - filtered: test_serial
+RESULT: FILTERED N/M suites ran (not run: test_admin_radio test_atak …) - filtered: test_serial

Also applies to: 738-738

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/copilot-instructions.md at line 735, Replace the literal suite
counts in the documented RESULT examples on lines 735 and 738 with the symbolic
N/M placeholder used on line 726, while preserving the existing result wording
and statuses.

Source: Coding guidelines


# FILTERED: single suite run completed cleanly
RESULT: FILTERED 1/24 suites ran (not run: test_admin_radio test_atak …) - filtered: test_serial [canonical: 1/24]
RESULT: FILTERED 1/24 suites ran (not run: test_admin_radio test_atak …) - filtered: test_serial
```

> **Copilot interface note:** When running tests via the Copilot chat interface, edits made through the chat may not be reflected in the on-disk files that the test binary reads. If tests pass in chat but fail locally (or vice versa), verify the files on disk match what you expect before trusting the result. Always confirm with a local terminal run.
Expand Down
72 changes: 41 additions & 31 deletions .github/workflows/test_native.yml
Original file line number Diff line number Diff line change
Expand Up @@ -23,54 +23,65 @@ env:
LCOV_CAPTURE_FLAGS: --quiet --capture --include "${PWD}/src/*" --exclude '*/src/mesh/generated/*' --directory .pio/build/coverage/src --base-directory "${PWD}"

jobs:
# Guard the registered native-suite total. `platformio test` discovers and runs whatever
# test_* directories exist, so it never notices when test/native-suite-count drifts from the
# actual directory count (a suite added without registering it, or the file left stale). That
# reconciliation only lives in bin/run-tests.sh, which CI does not invoke - so mirror the exact
# check here and fail the PR on a mismatch, keeping the manual count honest.
suite-count-check:
name: Native Suite Count
# Tripwire against the native suite set shrinking by accident. `platformio test` discovers and
# runs whatever test_* directories exist, and bin/run-tests.sh derives its expected count from
# the same walk - so a suite directory lost in a bad rebase or an overzealous cleanup just means
# fewer suites run, and every remaining check stays green. Compare the test_* directory list
# against the PR's merge base and fail when a suite vanished without the PR saying so: a removed
# suite's name must appear in the PR title, the PR body, or a commit message in the PR's range.
# A deliberate removal satisfies that by stating what it removes; an accidental loss cannot.
# Only pull_request runs have a base to compare against (and PRs are where accidents arrive);
# every other event skips. No job depends on this one: a skipped job would skip its dependents,
# and the expensive jobs should not wait on a full-history clone.
suite-shrinkage-check:
name: Native Suite Shrinkage
if: github.event_name == 'pull_request'
runs-on: ubuntu-slim
permissions:
contents: read
steps:
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7
with:
persist-credentials: false
# Full history: the merge base must be computed, not guessed from a possibly stale
# event payload, and the acknowledgment scan reads every commit message in the range.
fetch-depth: 0

- name: Reconcile native-suite-count with test/ directories
- name: Fail if a test_* suite vanished unacknowledged
shell: bash
# PR title/body are attacker-controlled text; they reach the script through env: only,
# never spliced into the shell source (same rule as the suite-order seed below).
env:
BASE_REF: ${{ github.base_ref }}
PR_TITLE: ${{ github.event.pull_request.title }}
PR_BODY: ${{ github.event.pull_request.body }}
run: |
set -euo pipefail
count_file="test/native-suite-count"
if [[ ! -f $count_file ]]; then
echo "::error title=Missing native-suite-count::$count_file not found - it must record the number of test_* suite directories."
exit 1
git fetch --quiet origin "$BASE_REF"
base=$(git merge-base FETCH_HEAD HEAD)
# Same canonical set every other consumer derives: directories named test_* directly
# under test/, read from the git trees so the comparison is exact at both endpoints.
list_suites() { git ls-tree -d --name-only "$1" test/ | sed 's#^test/##' | grep '^test_' | sort; }
removed=$(comm -23 <(list_suites "$base") <(list_suites HEAD))
if [[ -z $removed ]]; then
echo "No suite removed: $(list_suites HEAD | wc -l) test_* directories, none lost since merge base ${base:0:8}."
exit 0
fi
# Same canonical set as bin/run-tests.sh: directories named test_* directly under test/.
expected_count=$(find test -maxdepth 1 -type d -name 'test_*' -printf '%f\n' | wc -l)
canonical_count=$(tr -d '[:space:]' <"$count_file")
if ! [[ $canonical_count =~ ^[0-9]+$ ]]; then
echo "::error title=Invalid native-suite-count::$count_file must contain a single integer, got '$canonical_count'."
exit 1
fi
echo "test/ directories: $expected_count"
echo "native-suite-count: $canonical_count"
if [[ $expected_count -ne $canonical_count ]]; then
if [[ $expected_count -gt $canonical_count ]]; then
hint="a suite was added - bump $count_file to $expected_count"
messages=$(git log --format=%B "$base..HEAD")
fail=0
while IFS= read -r suite; do
if printf '%s\n%s\n%s\n' "$PR_TITLE" "$PR_BODY" "$messages" | grep -qF "$suite"; then
echo "Removed suite $suite is named in the PR title/body or a commit message - acknowledged."
else
hint="a suite was removed - lower $count_file to $expected_count"
echo "::error title=Native suite vanished::test/$suite exists on the merge base but is gone from this PR, and nothing in the PR title, body, or commit messages mentions it. If the removal is deliberate, name $suite in the PR description or a commit message; if not, restore the directory - platformio test would silently run without it."
fail=1
fi
echo "::error title=native-suite-count mismatch::test/ has $expected_count suite directories but $count_file says $canonical_count ($hint)."
exit 1
fi
echo "native-suite-count matches the $expected_count suite directories."
done <<<"$removed"
exit $fail

simulator-tests:
name: Native Simulator Tests
runs-on: ubuntu-24.04-arm
needs: suite-count-check
steps:
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7
with:
Expand Down Expand Up @@ -169,7 +180,6 @@ jobs:
platformio-tests:
name: Native PlatformIO Tests
runs-on: ubuntu-24.04-arm
needs: suite-count-check
steps:
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7
with:
Expand Down
2 changes: 1 addition & 1 deletion AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ Sequence these; don't parallelize on the same port.
| `src/modules/` | Feature modules; `Telemetry/Sensor/` has 50+ I2C sensor drivers |
| `variants/` | 200+ hardware variant definitions (`variant.h` + `platformio.ini` per board) |
| `protobufs/` | `.proto` definitions; regenerate with `bin/regen-protos.sh` |
| `test/` | Firmware unit tests (count: `test/native-suite-count`; `./bin/run-tests.sh` preferred, falls back to `pio test -e native`) |
| `test/` | Firmware unit tests (count = the `test_*` dirs, detected on the fly; `./bin/run-tests.sh` preferred, falls back to `pio test -e native`) |
| [meshtastic-mcp](https://github.com/meshtastic/meshtastic-mcp) | Standalone MCP server + tiered pytest hardware harness (`unit/`, `mesh/`, `telemetry/`, `monitor/`, `recovery/`, `ui/`, `fleet/`, `admin/`, `provisioning/`) - registered here via `.mcp.json` |
| `.github/prompts/` | Copilot prompt bodies (firmware scaffolding: new module / sensor / variant) |
| `.github/copilot-instructions.md` | **Primary agent instructions - read this** |
Expand Down
51 changes: 13 additions & 38 deletions bin/run-tests.sh
Original file line number Diff line number Diff line change
Expand Up @@ -163,20 +163,11 @@ export MESHTASTIC_TEST_STATE_SUMMARY="$STATE_SUMMARY"
$KEEP_STATE && export MESHTASTIC_TEST_KEEP_STATE=1
$WRITE_MANIFEST && export MESHTASTIC_TEST_KEEP_STATE=1

# Canonical suite set = the directories in test/. This is the source of truth for
# "what should run"; a filtered run only expects its filtered suite.
# Canonical suite set = the directories in test/, detected on the fly. This is the sole source
# of truth for "what should run"; a filtered run only expects its filtered suite.
mapfile -t ALL_SUITES < <(find test -maxdepth 1 -type d -name 'test_*' -printf '%f\n' | sort)
EXPECTED_COUNT=${#ALL_SUITES[@]}

# Canonical suite count - the registered total, maintained in test/native-suite-count.
# Update that file whenever a test suite is added or removed.
CANONICAL_COUNT_FILE="test/native-suite-count"
if [[ -f $CANONICAL_COUNT_FILE ]]; then
CANONICAL_COUNT=$(tr -d '[:space:]' <"$CANONICAL_COUNT_FILE")
else
CANONICAL_COUNT=""
fi

# Cached object-count for this env, written after each completed build (in the gitignored build
# dir). Used as the progress denominator: accurate for a full rebuild (every object recompiles),
# only a rough upper bound for an incremental run.
Expand Down Expand Up @@ -462,31 +453,15 @@ if ! grep -qE "$PASS_RE" "$LOG"; then
exit 1
fi

# Canonical-count rating suffix - appended to every verdict line so the result is always
# rated against the registered total, not just the directory count.
# If the two counts diverge (suite added/removed without updating native-suite-count), that
# is itself surfaced as AMBER before we reach any verdict.
canonical_rating() {
# Verdict-line suffix. The suite count itself is derived from the test_* directories on the fly
# (EXPECTED_COUNT above), so the only extra context a verdict needs is the shuffle seed - carried
# into the machine-readable line so a verdict is always replayable from it alone.
verdict_suffix() {
local rating=""
if [[ -n $CANONICAL_COUNT ]]; then
rating="[canonical: ${RAN_COUNT}/${CANONICAL_COUNT}]"
fi
# Carry the seed into the machine-readable line so a verdict is always replayable from it alone.
$SHUFFLE && rating="$rating [seed: $SEED]"
$SHUFFLE && rating="[seed: $SEED]"
echo "$rating"
}

# AMBER: directory count disagrees with native-suite-count - file needs updating.
if [[ -n $CANONICAL_COUNT && $EXPECTED_COUNT -ne $CANONICAL_COUNT ]]; then
echo ""
if [[ $EXPECTED_COUNT -gt $CANONICAL_COUNT ]]; then
echo "RESULT: AMBER test/ has $EXPECTED_COUNT suite directories but native-suite-count says $CANONICAL_COUNT - update test/native-suite-count after registering new suites"
else
echo "RESULT: AMBER test/ has $EXPECTED_COUNT suite directories but native-suite-count says $CANONICAL_COUNT - update test/native-suite-count after removing suites"
fi
exit 2
fi

# --- Shared-state axis --------------------------------------------------------
# Read what the per-suite wrapper recorded. Reported after the count checks so a structural problem
# still wins, and before the pass/fail verdict lines so the state summary always prints.
Expand Down Expand Up @@ -546,7 +521,7 @@ if [[ $IGNORED_COUNT -gt 0 ]]; then
echo ""
echo "$IGNORE_DETAIL"
echo ""
echo "RESULT: AMBER ${IGNORED_COUNT} test case(s) ignored $(canonical_rating)"
echo "RESULT: AMBER ${IGNORED_COUNT} test case(s) ignored $(verdict_suffix)"
exit 2
fi

Expand All @@ -558,7 +533,7 @@ if [[ -z $FILTER && $ACCOUNTED_COUNT -lt $EXPECTED_COUNT ]]; then
printf '%s\n' "${RAN_SUITES[@]}" "${SKIPPED_SUITES[@]}" | grep -qx "$s" || missing+=("$s")
done
echo ""
echo "RESULT: AMBER ${RAN_COUNT}/${EXPECTED_COUNT} suites ran (missing: ${missing[*]}) - all that ran passed $(canonical_rating)"
echo "RESULT: AMBER ${RAN_COUNT}/${EXPECTED_COUNT} suites ran (missing: ${missing[*]}) - all that ran passed $(verdict_suffix)"
exit 2
fi

Expand All @@ -572,7 +547,7 @@ if ((${#DIRTY_SUITES[@]} > 0)); then
echo ""
echo " -> declare these in test/state-manifest.tsv with a reason, or stop the write."
echo " -> ./bin/run-tests.sh --write-manifest prints the entries to paste."
echo "RESULT: AMBER ${#DIRTY_SUITES[@]} suite(s) left undeclared shared state $(canonical_rating)"
echo "RESULT: AMBER ${#DIRTY_SUITES[@]} suite(s) left undeclared shared state $(verdict_suffix)"
exit 2
fi

Expand All @@ -588,7 +563,7 @@ if ((${#SURVIVOR_SUITES[@]} > 0)); then
echo ""
echo " -> end every setup() branch with exit(UNITY_END()), not a bare UNITY_END()."
echo " -> ./bin/lint-unity-exit.sh test/**/*.cpp finds the sites; see test/README.md."
echo "RESULT: AMBER ${#SURVIVOR_SUITES[@]} suite(s) still running after the suite finished $(canonical_rating)"
echo "RESULT: AMBER ${#SURVIVOR_SUITES[@]} suite(s) still running after the suite finished $(verdict_suffix)"
exit 2
fi

Expand All @@ -599,10 +574,10 @@ if [[ -n $FILTER ]]; then
for s in "${ALL_SUITES[@]}"; do
printf '%s\n' "${RAN_SUITES[@]}" "${SKIPPED_SUITES[@]}" | grep -qx "$s" || not_run+=("$s")
done
echo "RESULT: FILTERED ${RAN_COUNT}/${EXPECTED_COUNT} suites ran (not run: ${not_run[*]}) - filtered: $FILTER $(canonical_rating)"
echo "RESULT: FILTERED ${RAN_COUNT}/${EXPECTED_COUNT} suites ran (not run: ${not_run[*]}) - filtered: $FILTER $(verdict_suffix)"
exit 3
fi

# GREEN: all canonical suites ran, all passed, no ignored test cases, nothing undeclared left behind.
echo "RESULT: GREEN ${RAN_COUNT}/${EXPECTED_COUNT} suites passed, all CLEAN $(canonical_rating)"
echo "RESULT: GREEN ${RAN_COUNT}/${EXPECTED_COUNT} suites passed, all CLEAN $(verdict_suffix)"
exit 0
5 changes: 3 additions & 2 deletions bin/test-lint-unity-exit.sh
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,9 @@
# anything" is what catches a rule that reports the right number of findings in the wrong places, or
# that collapses two findings on one line into one.
#
# Not a Unity suite and not counted in test/native-suite-count - same arrangement as
# bin/test-state-check.sh, and for the same reason: it asserts the behaviour of a process.
# Not a Unity suite and not a test_* directory, so outside the suite count run-tests.sh derives
# from test/ - same arrangement as bin/test-state-check.sh, and for the same reason: it asserts
# the behaviour of a process.
#
# Usage: ./bin/test-lint-unity-exit.sh (exit 0 = all fixtures behaved)

Expand Down
Loading
Loading