mirror/firmware
1
0 Fork 0
mirror of https://github.com/meshtastic/firmware.git synced 2026-09-16 08:30:04 -04:00
Code Issues Packages Projects Releases Wiki Activity

docs(agents): separate the suite-directory and test-function naming rules

Browse Source 
Review feedback on the previous commit was correct on both points.

The section called the test-function form snake_case while every example used
camelCase segments. The tree holds 743 test functions with an uppercase segment
and 675 without, so snake_case was wrong for more than half of them. Split the
two rules that were conflated: suite directories are strictly test_[a-z0-9_]+,
while test functions require only the test_ prefix and underscore separators,
with segment case free. States what is actually forbidden - dropping the prefix,
or collapsing the segments into one camelCase identifier.

The canonical-copy policy forbade restating the rule anywhere, then restated it
in AGENTS.md and .coderabbit.yaml. Name the YAML entry as the single permitted
copy, since a YAML instruction cannot follow a link, and reduce the AGENTS.md
bullet to a pointer.
This commit is contained in:
Thomas Göttgens committed 2026-09-04 14:27:25 +02:00
1 parent f776f0584c
commit b4fdd91682
3 files changed
+29 -21
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

.coderabbit.yaml
+16 -10
View File
@@ -33,16 +33,22 @@ reviews:
Ensure configurations include metadata found in other configs.
- path: test/**
instructions: >
Native C++ unit tests. Test names here are deliberately snake_case with a mandatory
test_ prefix - suite directories (test_<area>, lowercase) and test functions
(test_<subject>_<expectedBehavior>). This is NOT drift from the camelCase convention
used in src/, and it must not be "corrected". bin/run-tests.sh matches suite verdict
lines against test_[a-z0-9_]+ (lowercase only), so an uppercase suite directory is
reported as missing and downgrades the run to AMBER; RUN_TEST in test/TestUtil.h
passes #func to Unity, so the function name is the only attribution a CI failure
carries. Do NOT raise naming-convention comments on suite directories or test_*
functions. Helpers and fixtures inside a suite do follow the normal src/ conventions.
Authoritative rule: the "Test naming" section of .github/copilot-instructions.md.
Native C++ unit tests. The camelCase convention used in src/ does NOT apply here,
and this is deliberate, not drift. Two separate rules. Suite directories are
strictly test_[a-z0-9_]+, lowercase only. Test functions take a test_ prefix
followed by underscore-separated segments, and the case WITHIN a segment is free:
test_5byte_sequence_rejected and test_getRegion_returnsCorrectRegion_US are both
correct and both common. Only two things are forbidden for a test function -
dropping the test_ prefix, and collapsing the segments into a single camelCase
identifier. Rationale: bin/run-tests.sh matches suite verdict lines against
test_[a-z0-9_]+, so an uppercase suite directory is reported as missing and
downgrades the run to AMBER; RUN_TEST in test/TestUtil.h passes #func to Unity, so
the function name is the only attribution a CI failure carries. Do NOT raise
naming-convention comments on suite directories or test_* functions, and do NOT
flag a camelCase segment inside an otherwise underscore-separated test name.
Helpers and fixtures inside a suite do follow the normal src/ conventions.
Authoritative rule, which this entry mirrors: the "Test naming" section of
.github/copilot-instructions.md.
- path: "**/*.md"
instructions: >
Documentation does not live in this repo; it lives in
.github/copilot-instructions.md
+12 -10
View File
@@ -364,24 +364,26 @@ These apply to firmware source under `src/`. Code under `test/` is a deliberate
<a id="test-naming"></a>
#### Test naming - `snake_case`, not `camelCase`
#### Test naming - `test_` prefix and underscores, never one `camelCase` identifier
**This section is the single authoritative statement of the rule. Do not restate it in `AGENTS.md`, `CLAUDE.md`, or anywhere else - link here instead.**
**This section is the single authoritative statement of the rule. `AGENTS.md` and `CLAUDE.md` link here and must not restate it. The one permitted copy is the `test/**` entry in `.coderabbit.yaml`, because a YAML instruction cannot follow a link; keep it in sync with this section.**
Under `test/`, suite directories and test functions are `snake_case` with a mandatory `test_` prefix. This is not drift from the `src/` convention and not an oversight: it is required by the harness and by Unity. **A review comment asking for `camelCase` on a test suite directory or a `test_*` function is wrong, and should be rejected rather than acted on.**
Code under `test/` does not follow the `camelCase` rule above, and this is neither drift nor an oversight - the harness and Unity both depend on it. **A review comment asking for `camelCase` on a test suite directory or a `test_*` function is wrong, and should be rejected rather than acted on.**
| Thing | Form | Example |
| ------------------------------- | ----------------------------------- | ---------------------------------------------------- |
| Suite directory | `test_<area>`, lowercase | `test_gps_fix_hold/` |
| Test function | `test_<subject>_<expectedBehavior>` | `test_validateConfigRegion_unsetRegionReturnsTrue()` |
| Helpers/fixtures inside a suite | normal `src/` conventions | `makeFakePacket()`, `class FakeRadio` |
Two distinct rules, often conflated:
The `<subject>` segment often mirrors the `camelCase` symbol under test (`test_getRegion_returnsCorrectRegion_US`). What is fixed is the `test_` prefix and the `_` separators between segments, not the case inside a segment.
| Thing | Rule | Examples |
| ------------------------------- | ---------------------------------------------------------------------------- | ---------------------------------------------------------------------------- |
| Suite directory | Strictly `test_[a-z0-9_]+` - lowercase `snake_case`, no exceptions | `test_gps_fix_hold/`, `test_admin_radio/` |
| Test function | `test_` prefix, then `_`-separated segments. Case _within_ a segment is free | `test_5byte_sequence_rejected()`, `test_getRegion_returnsCorrectRegion_US()` |
| Helpers/fixtures inside a suite | normal `src/` conventions | `makeFakePacket()`, `class FakeRadio` |
For functions, what is fixed is the `test_` prefix and the underscores between segments - not the case inside a segment. Both `test_validateConfigRegion_unsetRegionReturnsTrue` (segment mirrors the `camelCase` symbol under test) and `test_5byte_sequence_rejected` (all lowercase) are correct and both are common in the tree. What is forbidden is dropping the prefix or collapsing the segments into a single `camelCase` identifier (`testValidateConfigRegionUnsetRegionReturnsTrue`).
Why it is fixed:
- **The harness discovers suites by prefix and parses their verdicts by regex.** `bin/run-tests.sh` enumerates suites with `find test -maxdepth 1 -type d -name 'test_*'`, then matches PlatformIO's per-suite result lines against `test_[a-z0-9_]+` - lowercase only. A suite directory with an uppercase letter is enumerated but never matched, so it is reported as _missing_ and the whole run downgrades from GREEN to AMBER.
- **The function name is the failure message.** `RUN_TEST` in `test/TestUtil.h` passes `#func` to `UnityDefaultTestRun()`, `testAssertEnvironmentIntact()` and `testStateCheckpoint()`, so the identifier is the only attribution a CI log carries for a failed assertion or a dirtied sandbox. The `_`-separated `subject_expectedBehavior` form is chosen so it reads as a sentence there.
- **The function name is the failure message.** `RUN_TEST` in `test/TestUtil.h` passes `#func` to `UnityDefaultTestRun()`, `testAssertEnvironmentIntact()` and `testStateCheckpoint()`, so the identifier is the only attribution a CI log carries for a failed assertion or a dirtied sandbox. The underscores are what make it readable there; a single `camelCase` run-on is not.
- **It is Unity's own convention**, shared with every other PlatformIO C++ project.
Renaming a suite directory to `camelCase` breaks the harness's suite accounting; renaming the functions destroys the readability of CI output. Leave both alone.
AGENTS.md
+1 -1
View File
@@ -80,7 +80,7 @@ Key rotation to never trigger casually: only the **full** factory reset (`factor
- **Run `trunk fmt` before proposing a commit.** The `trunk_check` CI gate will reject unformatted code. Claude Code runs it automatically via the PostToolUse hook in `.claude/settings.json`; trunk's launcher needs `curl` or `wget` to bootstrap its pinned CLI - see **Formatting & the trunk toolchain** in `.github/copilot-instructions.md` for the no-curl bootstrap procedure.
- **Never edit or commit files under `src/mesh/generated/`.** They are regenerated from the [`meshtastic/protobufs`](https://github.com/meshtastic/protobufs) repo by the `update_protobufs.yml` workflow (entry point: `bin/regen-protos.sh`). Local edits will be overwritten and create merge conflicts. If a `.proto` change is needed, open a PR against the protobufs repo first, then let the workflow re-sync this repo.
- **`confirm=True` on destructive MCP tools is a real gate, not a formality.** Don't bypass it via auto-approve settings.
- **Test names under `test/` are `snake_case` with a `test_` prefix on purpose.** The `camelCase` rule applies to `src/` only; suite directories and `test_*` functions must not be renamed to match it. Asking for that in review is wrong - the harness parses suite names with a lowercase-only regex, and Unity reports the function name as the failure message. Full rule and rationale: [**Test naming** in `.github/copilot-instructions.md`](.github/copilot-instructions.md#test-naming) - authoritative there, not restated here.
- **Test names under `test/` are not `camelCase`, deliberately.** The `src/` naming rule does not apply there, and suite directories and `test_*` functions must not be renamed to match it. Rule and rationale: [**Test naming** in `.github/copilot-instructions.md`](.github/copilot-instructions.md#test-naming) - authoritative there, not restated here.
- **Keep code comments minimal - one or two lines, max.** Comment only when the _why_ isn't obvious from the code; never restate what the next line does. No multi-paragraph block comments explaining straightforward changes. The diff and commit message carry the rationale; the code carries the behavior.
- **Documentation does not live in this repo. Do not add it here.** This repository holds firmware code. There is no `docs/` directory - the design documents that used to sit there were published to [meshtastic/meshtastic](https://github.com/meshtastic/meshtastic) in #11488 and the directory was deleted - and it must not come back. Do not create a `.md` file to describe a feature, a configuration surface, an API, a wire format, or a design; write it in the docs repo and link that PR instead. Never leave a write-up behind in the tree: no investigation notes, no mitigation plans, no migration checklists, no "how we got here" narrative, no summaries of what a change did. That is what the PR description and the commit message are for, and they are the only place it belongs. When you do write documentation upstream, write a technical manual, not a novel - what the feature does, the settings it exposes in the user's terms, and the exact API or protocol a client speaks. No story of the debugging journey, no rationale essays, no changelog prose. Concise and factual, as short as the facts allow.
- **Never compare against `millis()` directly. Use `Throttle`.** `src/mesh/Throttle.h` is the sanctioned way to ask about time, and CI enforces this (`millis-deadline-check` in `.github/workflows/test_native.yml` fails the PR on a new `millis() >` / `< millis()` comparison).