Skip to content

fix(test): restore the sendAckNak overrides broken by #10767 - #11626

Merged
thebentern merged 1 commit into
developfrom
fix/relay-ack-test-overrides
Aug 27, 2026
Merged

fix(test): restore the sendAckNak overrides broken by #10767#11626
thebentern merged 1 commit into
developfrom
fix/relay-ack-test-overrides

Conversation

@thebentern

@thebentern thebentern commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

#10767 added a relaySource parameter to the RoutingModule::sendAckNak virtual, but the five test mocks that derive from RoutingModule still declared the six-parameter signature with override. Nothing overrides the new virtual, so all five suites fail to compile and test-native / Native PlatformIO Tests has been red on develop since the merge:

test/test_reliable_ack_matrix/test_main.cpp:167:10: error: 'void MockRoutingModule::sendAckNak(
  meshtastic_Routing_Error, NodeNum, PacketId, ChannelIndex, uint8_t, bool)'
  marked 'override', but does not override
Environment    Test                      Status    Duration
-------------  ------------------------  --------  ------------
coverage       test_reliable_ack_matrix  ERRORED   00:00:11.848
coverage       test_mqtt                 ERRORED   00:00:12.001
coverage       test_nexthop_routing      ERRORED   00:00:11.738
coverage       test_packet_signing       ERRORED   00:00:11.762
coverage       test_mesh_module          ERRORED   00:00:11.854

The compile failure also cascaded: no suite emitted results, so the test-attribution gate reported 73 empty suites and lcov found no .gcda files.

What this does

Widens the five mocks to the new signature. test_reliable_ack_matrix, test_mqtt, test_nexthop_routing, test_packet_signing, test_mesh_module.

Carries has_rx_rssi with rx_rssi in allocAckNak(). rx_rssi has explicit presence (see #11271); copying only the value left has_rx_rssi false, so nanopb dropped the field at encode time and the phone never saw the relayer's RSSI that #10767 set out to deliver. relay_node and rx_snr are unconditional and were arriving fine.

Adds the coverage that would have caught both.

  • test_reliable_ack_matrix now asserts that the overheard rebroadcast is handed to sendAckNak as the relay source, with its relay_node / rx_rssi / rx_snr intact - on both the decodable path and the opaque #11502 ingress short-circuit - and that a normal want-ack ACK claims no relayer.
  • test_mesh_module drives a real RoutingModule and asserts the relay fields survive all the way to the phone, including has_rx_rssi; a second test asserts an ack with no relay source reaches the phone with the relay fields clear.

🤝 Attestations

  • I have tested that my proposed changes behave as described.
  • I have tested that my proposed changes do not cause any obvious regressions on the following devices:
    • Heltec (Lora32) V3
    • LilyGo T-Deck
    • LilyGo T-Beam
    • RAK WisBlock 4631
    • Seeed Studio T-1000E tracker card
    • Other (native test suite only - no device-facing behavior change beyond the has_rx_rssi presence flag)

Summary by CodeRabbit

  • Bug Fixes

    • Improved relay information in ACK/NAK packets by preserving whether received RSSI data is available.
    • Relay node, RSSI, and SNR details are now correctly delivered for applicable acknowledged messages.
  • Tests

    • Added coverage verifying relay information is included when available and omitted when no relay source exists.
    • Expanded reliability tests across rebroadcast and ingress scenarios.

#10767 added a relaySource parameter to the RoutingModule::sendAckNak
virtual, but the five test mocks that derive from RoutingModule still
declared the six-parameter signature with `override`. Nothing overrides
the new virtual, so all five suites fail to compile and the native test
job has been red on develop since the merge:

  test/test_reliable_ack_matrix/test_main.cpp:167:10: error: 'void
  MockRoutingModule::sendAckNak(meshtastic_Routing_Error, NodeNum,
  PacketId, ChannelIndex, uint8_t, bool)' marked 'override', but does
  not override

Widen the five mocks to the new signature.

Also carry has_rx_rssi with rx_rssi in allocAckNak(). rx_rssi has
explicit presence, so copying only the value left has_rx_rssi false and
nanopb dropped the field at encode time - the phone never saw the
relayer's RSSI that #10767 set out to deliver.

Cover both: test_reliable_ack_matrix asserts the overheard rebroadcast
is handed through as the relay source on the decodable path and the
opaque #11502 ingress path, and that no other ACK/NAK claims a relayer;
test_mesh_module drives a real RoutingModule and asserts the relay
fields, has_rx_rssi included, survive all the way to the phone.
@github-actions

Copy link
Copy Markdown
Contributor

⚡ Try this PR in the Web Flasher

Note

Building this pull request… the flash button, badges and supported-board
list will appear here automatically once CI finishes.

@coderabbitai

coderabbitai Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: fa3b2f5c-9129-4354-b710-0eb323ea35f2

📥 Commits

Reviewing files that changed from the base of the PR and between a8934a1 and 0be9288.

📒 Files selected for processing (6)
  • src/mesh/MeshModule.cpp
  • test/test_mesh_module/test_main.cpp
  • test/test_mqtt/MQTT.cpp
  • test/test_nexthop_routing/test_main.cpp
  • test/test_packet_signing/test_main.cpp
  • test/test_reliable_ack_matrix/test_main.cpp

📝 Walkthrough

Walkthrough

The ACK/NAK packet now copies the relay source RSSI presence flag. Mesh and reliable-ACK tests verify relay attribution and unset relay fields. Test routing overrides accept the updated method parameters.

Changes

ACK/NAK relay attribution

Layer / File(s) Summary
Relay source propagation and direct coverage
src/mesh/MeshModule.cpp, test/test_mesh_module/test_main.cpp
allocAckNak copies has_rx_rssi from the relay source. Mesh tests verify relay node, RSSI presence, RSSI, and SNR propagation, plus default fields without a relay source.
Reliable ACK relay assertions
test/test_reliable_ack_matrix/test_main.cpp
The reliable ACK matrix records relay-source fields and checks normal ACK, rebroadcast ACK, and opaque ingress ACK behavior.
Test routing override alignment
test/test_mqtt/MQTT.cpp, test/test_nexthop_routing/test_main.cpp, test/test_packet_signing/test_main.cpp
Test routing-module overrides accept the updated sendAckNak parameters and preserve existing behavior.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: caveman99, nomdetom

✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/relay-ack-test-overrides

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@thebentern thebentern added the bugfix Pull request that fixes bugs label Aug 27, 2026
@thebentern
thebentern merged commit 9a59e90 into develop Aug 27, 2026
52 of 54 checks passed
@caveman99
caveman99 deleted the fix/relay-ack-test-overrides branch August 27, 2026 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix Pull request that fixes bugs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant