Merge pull request #10619 from NomDeTom/test_fixes

Some fixes and tidies for testing both online and in unit_tests

.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_admin_radio/` - LoRa region/config validation and AdminModule dispatch
-- `test_mqtt/` - MQTT integration
-- `test_radio/` - Radio interface
-- `test_mesh_module/` - Module framework
-- `test_meshpacket_serializer/` - Packet serialization
-- `test_transmit_history/` - Retransmission tracking
 - `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`
 

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                                                                                                       |
+| Action                           | Command                                                                                                                                                                                                        |
-| -------------------------------- | ------------------------------------------------------------------------------------------------------------- |
+| -------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
-| Build a firmware variant         | `pio run -e <env>` (e.g. `pio run -e rak4631`, `pio run -e heltec-v3`)                                        |
+| Build a firmware variant         | `pio run -e <env>` (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`) |
+| 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 <env> -t clean && pio run -e <env>`                                                               |
+| Clean + rebuild                  | `pio run -e <env> -t clean && pio run -e <env>`                                                                                                                                                                |
-| Flash a device                   | `pio run -e <env> -t upload --upload-port <port>` (or use the `pio_flash` MCP tool)                           |
+| Flash a device                   | `pio run -e <env> -t upload --upload-port <port>` (or use the `pio_flash` MCP tool)                                                                                                                            |
-| Run firmware unit tests (native) | `pio test -e native`                                                                                          |
+| 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`                                                                                   |
+| Run MCP hardware tests           | `./mcp-server/run-tests.sh`                                                                                                                                                                                    |
-| Live TUI test runner             | `mcp-server/.venv/bin/meshtastic-mcp-test-tui`                                                                |
+| Live TUI test runner             | `mcp-server/.venv/bin/meshtastic-mcp-test-tui`                                                                                                                                                                 |
-| Format before commit             | `trunk fmt`                                                                                                   |
+| Format before commit             | `trunk fmt`                                                                                                                                                                                                    |
-| Regenerate protobuf bindings     | `bin/regen-protos.sh`                                                                                         |
+| Regenerate protobuf bindings     | `bin/regen-protos.sh`                                                                                                                                                                                          |
-| Generate CI matrix               | `./bin/generate_ci_matrix.py all [--level pr]`                                                                |
+| 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                                                                                         |

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;

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];

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);

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);

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;

test/README.md

@@ -15,6 +15,38 @@ 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.
+
+**Viewing verbose test output without truncation (e.g. `TEST_MESSAGE` group headers):**
+
+```bash
+/tmp/meshtastic-pio-venv/bin/python -m platformio test -e coverage --filter test_mesh_beacon -vv 2>&1 | grep -v "[[:space:]]SKIPPED$"
+```
+
+The `-vv` flag makes Unity emit `INFO:` lines from `TEST_MESSAGE` calls; piping through `grep -v SKIPPED` removes the noise from platform feature gates while keeping all PASS/FAIL/INFO lines visible.
+
+**`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.
@@ -82,12 +114,15 @@ One file per suite. No per-test `platformio.ini` is needed — tests build under
 #include "gps/RTC.h"
 #include "mesh/NodeDB.h"
 #include "modules/YourModule.h"
-#include <cstdio>
+#include <cstdio>    // required for printf() — used for blank-line group separators
 #include <cstring>
 #include <memory>
 
 // --- Test output helpers ---
-// Unity swallows printf/stdout. Only TEST_MESSAGE() output appears in results.
+// printf() writes directly to stdout and appears in -vv output as a plain line (no prefix).
+// Use it for blank-line group separators: printf("\n");
+// TEST_MESSAGE() emits a "file:line:INFO: <text>" line — visible at -vv and above.
+// Use TEST_MSG_FMT for formatted diagnostic lines inside tests.
 #define MSG_BUF_LEN 200
 #define TEST_MSG_FMT(fmt, ...) do { \
     char _buf[MSG_BUF_LEN]; \
@@ -112,6 +147,9 @@ void setup()
 {
     initializeTestEnvironment();   // MUST call — sets up RTC, OSThread, console
     UNITY_BEGIN();
+
+    printf("\n=== Example group ===\n");           // header line to help find tests
+
     RUN_TEST(test_example);
     exit(UNITY_END());             // exit() required — Unity runner expects it
 }
@@ -185,20 +223,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 +336,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)

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);

test/test_mqtt/MQTT.cpp

@@ -27,12 +27,6 @@
 #include <utility>
 #include <variant>
 
-#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.

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"