diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index d165f2cdb..edf1760ea 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -609,20 +609,35 @@ Most workflows can be triggered manually via `workflow_dispatch` for testing. ### Native unit tests (C++) -Unit tests in `test/` directory with 12 test suites: +Unit tests in `test/` directory with 17 test suites: -- `test_crypto/` - Cryptography -- `test_mqtt/` - MQTT integration -- `test_radio/` - Radio interface -- `test_mesh_module/` - Module framework -- `test_meshpacket_serializer/` - Packet serialization -- `test_transmit_history/` - Retransmission tracking +- `test_admin_radio/` - LoRa region/config validation and AdminModule dispatch - `test_atak/` - ATAK integration +- `test_crypto/` - Cryptography - `test_default/` - Default configuration - `test_http_content_handler/` - HTTP handling +- `test_mac_from_string/` - MAC address parsing +- `test_mesh_module/` - Module framework +- `test_meshpacket_serializer/` - Packet serialization +- `test_mqtt/` - MQTT integration +- `test_packet_history/` - Packet history tracking +- `test_position_precision/` - Position precision helpers +- `test_radio/` - Radio interface - `test_serial/` - Serial communication +- `test_traffic_management/` - Traffic management +- `test_transmit_history/` - Retransmission tracking +- `test_type_conversions/` - NodeDB v25 type conversion (bitfield round-trips, NodeInfoLite) +- `test_utf8/` - UTF-8 utilities -Run with: `pio test -e native` +Run command (preferred — avoids pipe-buffering and the Ubuntu externally-managed-environment error): + +```bash +~/.platformio/penv/bin/python -m platformio test -e native -f test_your_suite > /tmp/test_out.txt 2>&1 +grep -E 'error:|PASS|FAIL|succeeded|failed' /tmp/test_out.txt +tail -15 /tmp/test_out.txt +``` + +Do **not** use `pio test … | tail -N` — it discards build errors and shows stale cached results. Do **not** use `pio test … | grep` — line-buffering makes the terminal appear hung until the process exits. Redirect to a file first, then grep. Simulation testing: `bin/test-simulator.sh` diff --git a/AGENTS.md b/AGENTS.md index 82912f252..c140d6648 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -10,18 +10,18 @@ This file (`AGENTS.md`) is a short pointer + quick reference for agents that don ## Quick command reference -| Action | Command | -| -------------------------------- | ------------------------------------------------------------------------------------------------------------- | -| Build a firmware variant | `pio run -e ` (e.g. `pio run -e rak4631`, `pio run -e heltec-v3`) | -| Build native macOS host binary | `pio run -e native-macos` (Homebrew prereqs + CH341 LoRa setup in `variants/native/portduino/platformio.ini`) | -| Clean + rebuild | `pio run -e -t clean && pio run -e ` | -| Flash a device | `pio run -e -t upload --upload-port ` (or use the `pio_flash` MCP tool) | -| Run firmware unit tests (native) | `pio test -e native` | -| Run MCP hardware tests | `./mcp-server/run-tests.sh` | -| Live TUI test runner | `mcp-server/.venv/bin/meshtastic-mcp-test-tui` | -| Format before commit | `trunk fmt` | -| Regenerate protobuf bindings | `bin/regen-protos.sh` | -| Generate CI matrix | `./bin/generate_ci_matrix.py all [--level pr]` | +| Action | Command | +| -------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| Build a firmware variant | `pio run -e ` (e.g. `pio run -e rak4631`, `pio run -e heltec-v3`) | +| Build native macOS host binary | `pio run -e native-macos` (Homebrew prereqs + CH341 LoRa setup in `variants/native/portduino/platformio.ini`) | +| Clean + rebuild | `pio run -e -t clean && pio run -e ` | +| Flash a device | `pio run -e -t upload --upload-port ` (or use the `pio_flash` MCP tool) | +| Run firmware unit tests (native) | `~/.platformio/penv/bin/python -m platformio test -e native > /tmp/test_out.txt 2>&1` then `grep -E 'error:\|PASS\|FAIL\|succeeded\|failed' /tmp/test_out.txt` (redirect first — piping causes line-buffering) | +| Run MCP hardware tests | `./mcp-server/run-tests.sh` | +| Live TUI test runner | `mcp-server/.venv/bin/meshtastic-mcp-test-tui` | +| Format before commit | `trunk fmt` | +| Regenerate protobuf bindings | `bin/regen-protos.sh` | +| Generate CI matrix | `./bin/generate_ci_matrix.py all [--level pr]` | ## MCP server (device + test automation) @@ -108,7 +108,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 (12 suites; `pio test -e native`) | +| `test/` | Firmware unit tests (17 suites; `pio test -e native`) | | `mcp-server/` | Python MCP server + pytest hardware integration tests | | `mcp-server/tests/` | Tiered pytest suite: `unit/`, `mesh/`, `telemetry/`, `monitor/`, `recovery/`, `ui/`, `fleet/`, `admin/`, `provisioning/` | | `.claude/commands/` | Claude Code slash command bodies | diff --git a/src/mesh/MeshRadio.h b/src/mesh/MeshRadio.h index f5da7cec2..d04f827c7 100644 --- a/src/mesh/MeshRadio.h +++ b/src/mesh/MeshRadio.h @@ -77,6 +77,7 @@ extern const RegionInfo regions[]; extern const RegionInfo *myRegion; extern void initRegion(); +extern const RegionInfo *getRegion(meshtastic_Config_LoRaConfig_RegionCode code); // Valid LoRa spread factor range and defaults constexpr uint8_t LORA_SF_MIN = 5; diff --git a/src/mesh/RadioInterface.cpp b/src/mesh/RadioInterface.cpp index 4493a58d9..7f321b0dd 100644 --- a/src/mesh/RadioInterface.cpp +++ b/src/mesh/RadioInterface.cpp @@ -834,6 +834,16 @@ bool RadioInterface::validateConfigRegion(const meshtastic_Config_LoRaConfig &lo { const RegionInfo *newRegion = getRegion(loraConfig.region); + // Reject unrecognized region codes (getRegion returns UNSET sentinel for unknown codes) + if (newRegion->code != loraConfig.region) { + char err_string[160]; + snprintf(err_string, sizeof(err_string), "Region code %d is not recognized", loraConfig.region); + LOG_ERROR("%s", err_string); + RECORD_CRITICALERROR(meshtastic_CriticalErrorCode_INVALID_RADIO_SETTING); + sendErrorNotification(err_string); + return false; + } + // If you are not licensed, you can't use ham regions. if (newRegion->profile->licensedOnly && !devicestate.owner.is_licensed) { char err_string[160]; diff --git a/src/modules/SerialModule.cpp b/src/modules/SerialModule.cpp index 0266bcec5..f0d1278aa 100644 --- a/src/modules/SerialModule.cpp +++ b/src/modules/SerialModule.cpp @@ -94,7 +94,7 @@ bool SerialModule::isValidConfig(const meshtastic_ModuleConfig_SerialConfig &con const char *warning = "Invalid Serial config: override console serial port is only supported in NMEA and CalTopo output-only modes."; LOG_ERROR(warning); -#if !IS_RUNNING_TESTS +#ifndef PIO_UNIT_TESTING meshtastic_ClientNotification *cn = clientNotificationPool.allocZeroed(); cn->level = meshtastic_LogRecord_Level_ERROR; cn->time = getValidTime(RTCQualityFromNet); diff --git a/src/mqtt/MQTT.cpp b/src/mqtt/MQTT.cpp index 46ad952f4..e36e6edf7 100644 --- a/src/mqtt/MQTT.cpp +++ b/src/mqtt/MQTT.cpp @@ -466,7 +466,7 @@ MQTT::MQTT() : concurrency::OSThread("mqtt"), mqttQueue(MAX_MQTT_QUEUE) enabled = true; runASAP = true; reconnectCount = 0; -#if !IS_RUNNING_TESTS +#ifndef PIO_UNIT_TESTING publishNodeInfo(); #endif } @@ -674,7 +674,7 @@ bool MQTT::isValidConfig(const meshtastic_ModuleConfig_MQTTConfig &config, MQTTC const char *warning = "Could not reach the MQTT server. Settings will be saved, but please verify the server " "address and credentials."; LOG_WARN(warning); -#if !IS_RUNNING_TESTS +#ifndef PIO_UNIT_TESTING meshtastic_ClientNotification *cn = clientNotificationPool.allocZeroed(); if (cn) { cn->level = meshtastic_LogRecord_Level_WARNING; @@ -690,7 +690,7 @@ bool MQTT::isValidConfig(const meshtastic_ModuleConfig_MQTTConfig &config, MQTTC #else const char *warning = "Invalid MQTT config: proxy_to_client_enabled must be enabled on nodes that do not have a network"; LOG_ERROR(warning); -#if !IS_RUNNING_TESTS +#ifndef PIO_UNIT_TESTING meshtastic_ClientNotification *cn = clientNotificationPool.allocZeroed(); cn->level = meshtastic_LogRecord_Level_ERROR; cn->time = getValidTime(RTCQualityFromNet); @@ -706,7 +706,7 @@ bool MQTT::isValidConfig(const meshtastic_ModuleConfig_MQTTConfig &config, MQTTC if (defaultServer && !IS_ONE_OF(parsed.serverPort, PubSubConfig::defaultPort, PubSubConfig::defaultPortTls)) { const char *warning = "Invalid MQTT config: default server address must not have a port specified"; LOG_ERROR(warning); -#if !IS_RUNNING_TESTS +#ifndef PIO_UNIT_TESTING meshtastic_ClientNotification *cn = clientNotificationPool.allocZeroed(); cn->level = meshtastic_LogRecord_Level_ERROR; cn->time = getValidTime(RTCQualityFromNet); diff --git a/src/platform/portduino/PortduinoGlue.cpp b/src/platform/portduino/PortduinoGlue.cpp index 59d9f7526..c3cb1c780 100644 --- a/src/platform/portduino/PortduinoGlue.cpp +++ b/src/platform/portduino/PortduinoGlue.cpp @@ -562,7 +562,7 @@ void portduinoSetup() } getMacAddr(dmac); -#ifndef UNIT_TEST +#ifndef PIO_UNIT_TESTING if (dmac[0] == 0 && dmac[1] == 0 && dmac[2] == 0 && dmac[3] == 0 && dmac[4] == 0 && dmac[5] == 0) { std::cout << "*** Blank MAC Address not allowed!" << std::endl; std::cout << "Please set a MAC Address in config.yaml using either MACAddress or MACAddressSource." << std::endl; diff --git a/test/README.md b/test/README.md index 55dbd4775..cc3275ef4 100644 --- a/test/README.md +++ b/test/README.md @@ -15,6 +15,30 @@ pio test -e native -f test_your_module pio test -e native -f test_your_module -vvv ``` +**Never pipe through `| tail -N` to shorten output.** PlatformIO prints build errors at the top of output and test results at the bottom; `tail` will show stale cached results from a prior successful build while hiding the compile error that caused the current run to fail. + +**Preferred pattern — redirect to file, then grep:** + +```bash +# Redirect all output to a file; grep for errors and results after it exits +pio test -e native -f test_your_module > /tmp/test_out.txt 2>&1 +echo "exit: $?" +grep -E 'error:|PASS|FAIL|succeeded|failed' /tmp/test_out.txt +tail -15 /tmp/test_out.txt +``` + +Why: piping through `| grep` line-buffers the output and suppresses all progress until the process exits, making it look hung. The redirect approach lets the build stream normally while still giving you filtered results afterwards. + +**`externally-managed-environment` error on Ubuntu/Debian:** + +If `pio test` fails immediately with `error: externally-managed-environment`, the system `pio` binary is using the OS Python which newer distros lock down. Use PlatformIO's own venv instead: + +```bash +~/.platformio/penv/bin/python -m platformio test -e native -f test_your_module > /tmp/test_out.txt 2>&1 +grep -E 'error:|PASS|FAIL|succeeded|failed' /tmp/test_out.txt +tail -15 /tmp/test_out.txt +``` + ### Helper Scripts (Useful Shortcuts) These wrappers are handy when local host dependencies are missing or when you want repeatable commands. @@ -185,20 +209,34 @@ Subclass the module under test to make protected methods callable and private me class YourModuleTestShim : public YourModule { public: - // Expose protected methods + // Pull protected methods into public scope via using. + // IMPORTANT: using requires the method to be protected (or public) in the base — + // friend alone does NOT satisfy this. See pitfall #6. using YourModule::runOnce; using YourModule::someProtectedMethod; - // Access private members via friend (see below) + // Wrap private members with setter methods (friend grants direct access here). void setPrivateField(int x) { privateField = x; } }; ``` -In the module header, grant friend access under the `UNIT_TEST` define (set automatically by PlatformIO's test framework): +For methods you want to expose via `using`, use the conditional access-specifier pattern in the header — **not** plain `friend`: ```cpp // In YourModule.h, inside the class body: -#ifdef UNIT_TEST +#ifdef PIO_UNIT_TESTING + protected: +#else + private: +#endif + bool someMethod(); +``` + +For private _member variables_ that a shim setter needs to touch directly, `friend` is sufficient (no `using` involved): + +```cpp +// In YourModule.h, inside the class body: +#ifdef PIO_UNIT_TESTING friend class YourModuleTestShim; #endif ``` @@ -284,6 +322,21 @@ Fixed-size data structures (hash sets, ring buffers) overflow when tests inject **Fix:** Simulate multiple realistic time windows rather than one massive burst. Let adaptive mechanisms (if any) self-tune over several rolls. +### 6. Granting test access to private/protected members + +PlatformIO defines `PIO_UNIT_TESTING` during `pio test` builds. Several production headers (`TransmitHistory.h`, `CryptoEngine.h`, `MQTT.h`, `RTC.h`) use this to gate test-only visibility changes. PlatformIO also defines `UNIT_TEST` in the same builds for backward compatibility, but that spelling is deprecated — always use `PIO_UNIT_TESTING` in new code. The established pattern for exposing a private method to a test shim **without widening production visibility**: + +```cpp +#ifdef PIO_UNIT_TESTING + protected: +#else + private: +#endif + bool myMethod(); +``` + +**Critical C++ rule:** a `using` declaration in a derived class (e.g. `using Base::myMethod`) requires `myMethod` to be `protected` or `public` in the base — `friend` alone does **not** satisfy this. Adding `friend class TestShim` while leaving the method `private` will still fail to compile. Use the conditional access-specifier pattern above, not `friend`. + ## setUp/tearDown Checklist - [ ] Create and clear MockNodeDB (if needed) diff --git a/test/test_admin_radio/test_main.cpp b/test/test_admin_radio/test_main.cpp index e370f26b9..bfcc73b57 100644 --- a/test/test_admin_radio/test_main.cpp +++ b/test/test_admin_radio/test_main.cpp @@ -36,7 +36,6 @@ static MockMeshService *mockMeshService; // ----------------------------------------------------------------------- // getRegion() tests // ----------------------------------------------------------------------- -extern const RegionInfo *getRegion(meshtastic_Config_LoRaConfig_RegionCode code); static void test_getRegion_returnsCorrectRegion_US() { @@ -104,6 +103,29 @@ static void test_validateConfigRegion_unsetRegionReturnsTrue() TEST_ASSERT_TRUE(RadioInterface::validateConfigRegion(cfg)); } +static void test_validateConfigRegion_unknownCodeReturnsFalse() +{ + meshtastic_Config_LoRaConfig cfg = meshtastic_Config_LoRaConfig_init_zero; + cfg.region = (meshtastic_Config_LoRaConfig_RegionCode)255; + + devicestate.owner.is_licensed = false; + + // Unknown code is not in the regions table; getRegion() returns the UNSET sentinel, + // whose .code != 255, so validateConfigRegion should reject it. + TEST_ASSERT_FALSE(RadioInterface::validateConfigRegion(cfg)); +} + +static void test_validateConfigRegion_anotherUnknownCodeReturnsFalse() +{ + meshtastic_Config_LoRaConfig cfg = meshtastic_Config_LoRaConfig_init_zero; + cfg.region = (meshtastic_Config_LoRaConfig_RegionCode)99; + + devicestate.owner.is_licensed = true; + + // Unknown code should be rejected even when owner is licensed. + TEST_ASSERT_FALSE(RadioInterface::validateConfigRegion(cfg)); +} + // ----------------------------------------------------------------------- // Shadow tables for testing (preset lists → profiles → regions → lookup) // ----------------------------------------------------------------------- @@ -937,6 +959,8 @@ void setup() // validateConfigRegion() RUN_TEST(test_validateConfigRegion_validRegionReturnsTrue); RUN_TEST(test_validateConfigRegion_unsetRegionReturnsTrue); + RUN_TEST(test_validateConfigRegion_unknownCodeReturnsFalse); + RUN_TEST(test_validateConfigRegion_anotherUnknownCodeReturnsFalse); // Shadow table tests RUN_TEST(test_shadowTable_spacedProfileHasNonZeroSpacing); diff --git a/test/test_mqtt/MQTT.cpp b/test/test_mqtt/MQTT.cpp index 523e1d4ac..410f419ec 100644 --- a/test/test_mqtt/MQTT.cpp +++ b/test/test_mqtt/MQTT.cpp @@ -27,12 +27,6 @@ #include #include -#if defined(UNIT_TEST) -#define IS_RUNNING_TESTS 1 -#else -#define IS_RUNNING_TESTS 0 -#endif - namespace { // Minimal router needed to receive messages from MQTT. diff --git a/test/test_serial/SerialModule.cpp b/test/test_serial/SerialModule.cpp index 1bccf04a7..39992b3a2 100644 --- a/test/test_serial/SerialModule.cpp +++ b/test/test_serial/SerialModule.cpp @@ -5,12 +5,6 @@ #ifdef ARCH_PORTDUINO #include "configuration.h" -#if defined(UNIT_TEST) -#define IS_RUNNING_TESTS 1 -#else -#define IS_RUNNING_TESTS 0 -#endif - #if (defined(ARCH_ESP32) || defined(ARCH_NRF52) || defined(ARCH_RP2040)) && !defined(CONFIG_IDF_TARGET_ESP32S2) && \ !defined(CONFIG_IDF_TARGET_ESP32C3) #include "modules/SerialModule.h"