Repository navigation
fix(tests): pin the typed model-control route in three full-suite tests. Principle XI. - #6421
Conversation
…ts. Principle XI. #6199 moved slice and transmit controls onto typed requests that dispatch only to a connected radio with an open TX session. Three tests outside the focused set still described the retired route and failed on main's full suite at d3ab349: - fm_tone_presentation_contract checks that SliceModel publishes the normalized DTCS request and the Icom adapter applies it, instead of grepping RadioModel for the removed fmDtcsCommandIssued forward. - icom_family_test drives the operator mic edit on a socket-free rebuilt backend with its link marked up; the abandoned connect attempt leaves TX admission closed, as production does until onConnected(). - hl2_tci_signaling_test marks the HL2 link up before a TCI client promotes a slice, as a TCI client does on a live radio. Each assertion is unchanged and fails when its backend dispatch is removed. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
macOS run of this head, since check-macos compiles but does not run the suite: at The HL2 change reads right to me: the link flag is what 73, ON8ST |
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
Issue fit
Partially, by design — and the remainder is correctly disclosed. #6420 names four failures at d3ab349; this PR fixes three and attributes radio_setup_headphone_output_gate_test to the pre-existing #6418 (#6413's Radio Setup rework), which the issue body's own assertion text supports — it is a theme-token comparison, not a route failure. Leaving it out is right, not a gap.
I verified the root cause the body claims. RadioModel::dispatchTransmitControl (src/models/RadioModel.cpp:10000-10003) gates on !m_backend || !isConnected() || m_txSessionClosing, and IcomCivBackend::isConnected() (src/core/backends/icom/IcomCivBackend.cpp:1036) is return m_connected;. #6420's log carries the matching line — RadioModel: not connected, dropping slice 0 receive intent — so the diagnosis and the fix seam both hold up.
"Closes #6420" is fine: the full-suite fingerprint is the sorted failed-test name set, so the surviving #6418 failure opens a fresh sticky rather than reopening this one.
Each of the three replacements is a genuine pin, not an implementation tautology:
tests/fm_tone_presentation_contract_test.py:76—controlRequested({field, DtcsSetting{exists exactly once, atsrc/models/SliceModel.cpp:1140. The tempered regex at line 81 (case Field::FmDtcs: \{(?:(?!case Field::).)*setSliceFmDtcs\() matchesIcomCivBackend.cpp:4417-4423and there is only onecase Field::FmDtcsin the file, so it cannot drift onto a neighbouring case.import reis already present (line 14).tests/icom_family_test.cpp—case Field::MicGain: setMicGain(...)atIcomCivBackend.cpp:4485, andsetMicGainsetsm_micGainReportedat 4677-4678. ReturningUnsupportedthere does leave the assertion failing, as the negative-control box claims.tests/hl2_tci_signaling_test.cpp—case Field::TxSliceatHl2Backend.cpp:3236callssetTxSlice, which is what the two promote assertions read.
Scope
| File | What it changes | Claimed? | Verdict |
|---|---|---|---|
tests/fm_tone_presentation_contract_test.py |
Retargets the DTCS source-text pin from RadioModel.cpp to SliceModel.cpp + IcomCivBackend.cpp |
Yes (body bullet 1) | In scope |
tests/hl2_tci_signaling_test.cpp |
Adds Hl2TxGateTestAccess (reuses the existing friend) and marks the link up around the promote assertions |
Yes (body bullet 3) | In scope |
tests/icom_family_test.cpp |
Adds setConnected to the existing test-access struct; reruns the mic edit on a rebuildBackendForTest backend |
Yes (body bullet 2) | In scope |
Everything in the diff is explained by #6420. No production code, no tests.cmake change, no new public or protocol surface, no capability field, no settings key, no CHANGELOG.md entry (correct — it's a release-prep file). No - line removes a guard: the only deletions are the three stale fail() strings being replaced. No preference change — this is test-precondition repair with an authority (the #6420 run) behind it.
Socket tests: none added, modified or removed. hl2_tci_signaling_test names TciServer/TciClient, but the client is a heap object driven by emit client->textMessageReceived(...) — nothing binds, listens or connects, and this PR's added lines are setConnected calls only. The security audit independently confirmed this.
Security audit (security-audit / CodeGuard): no findings. All 39 surfaced hits are refuted. CS-VLN-CMDI-BACKTICK on IRadioBackend.h, RadioCapabilities.h, AnanBackend.cpp:1066 and aetherd-touchpoint-tags.json are lexical matches on Markdown-quoted protocol verbs inside // comments and JSON "note" strings — AnanBackend.cpp:1066 is the comment sends `mixer lineout gain 50` in setLineoutGain(), whose body is std::clamp plus a signal emit. CS-VLN-SSRF-HEX at FlexBackend.cpp:1266 is handle == QLatin1String("0x00000000"), the SmartSDR placeholder-amplifier sentinel — a string compare, nothing resolved. The PII hits are TEST-NET-1 fixture addresses and the 1234-5678-9012-3456 Flex serial. Note that 17 of the 20 listed files do not intersect this diff at all; that file list is a whole-file scan of main's tip.
Blockers
None.
Nits
-
tests/icom_family_test.cpp:1022-1025— the new pin can silently not run, and the check message asserts more than the return value can establish. This is the one I'd fix:check(model.rebuildBackendForTest(QStringLiteral("icom")), "a socket-free Icom backend replaces the unconnected one"); auto* live = dynamic_cast<icom::IcomCivBackend*>(model.backend()); if (live) {
rebuildBackendForTestisreturn m_backend != nullptr;(src/models/RadioModel.cpp:10091-10098), andsetupBackend()falls back toFlexBackendfor any unrecognized family — the comment atRadioModel.cpp:445-448says so explicitly ("flex(the default, and any unrecognized value)"). So thecheckpasses on a Flex backend, thedynamic_castyields null,if (live)skips, and the mic-gain assertion — the thing this PR exists to pin — vanishes with the test still green. The adjacent pre-existing block does it correctly (check(rebuilt != nullptr, ...)beforeif (rebuilt)), and so does your own hl2 hunk attests/hl2_tci_signaling_test.cpp:681-683. Drop-in:auto* live = dynamic_cast<icom::IcomCivBackend*>(model.backend()); check(live != nullptr, "the socket-free rebuild produced an Icom backend"); if (live) {
-
tests/icom_family_test.cpp:50-60— the fabricatedIcomSessionis surplus. The gate isisConnected(), which is justm_connected, sobackend.m_connected = connectedalone opens dispatch. Downstream,setMicGainresolvesm_session ? m_session->civAddress() : 0xA4(IcomCivBackend.cpp:4679) andIcomSession::Params::civAddressdefaults to0xA4(IcomSession.h:67) — identical either way. Harmless as written (the default ctor is: QObject(parent) {}, and~IcomSession() { stop(); }is fully null-guarded, so thereset()is safe), but it makesm_session != nullptrtrue for any other code the fixture reaches, which is a wider fiction than the test needs. -
tests/icom_family_test.cpp:1013-1021— dangling comment. The paragraph ending "reopen admission the socket-free way and mark the link up" is the last thing inside theif (rebuilt)block but describes the code that follows after the block closes at 1021. Moving it below the brace would read correctly. -
The body's "Every assertion message and expected value is unchanged" is not accurate for the contract test. Two
fail()messages were rewritten and a third check was added (tests/fm_tone_presentation_contract_test.py:82-83). Bullet 1 describes this honestly, so it's an internal inconsistency in the summary rather than a concealment — worth tightening if the body gets edited. -
tests/fm_tone_presentation_contract_test.py:76-83— the replacement pins the two ends of the route but no longer the hop between them. The old version greppedRadioModel.cppform_backend->setSliceFmDtcs(; the new one checks thatSliceModelemits and that the Icom adapter handles. Delete theconnect(s, &SliceModel::controlRequested, this, &RadioModel::dispatchSliceControl, type)wiring atsrc/models/RadioModel.cpp:9834-9835and both greps still pass. Presumably one of the 23 focused #6199 tests covers that hop — worth naming which, since this file is the full suite's stated evidence for the route.
What I tried to break
- Does it compile?
friend struct Hl2TxGateTestAccessalready exists atsrc/core/backends/hl2/Hl2Backend.h:279, so the new struct needs no production hook — the comment's claim is accurate.m_connectedis atHl2Backend.h:418.std::make_unique<IcomSession>()needs a default ctor, andIcomSession.h:70hasexplicit IcomSession(QObject* parent = nullptr)with a trivial body atIcomSession.cpp:49.m_sessionisstd::unique_ptr<IcomSession>(IcomCivBackend.h:338),<memory>andIcomSession.hare both newly included. - ODR on
Hl2TxGateTestAccess, now defined in three test files (hl2_tci_signaling_test.cpp:654,hl2_tx_gate_test.cpp:69,hl2_backend_seam_test.cpp:47).tests/tests.cmake:6663buildshl2_tci_signaling_testas a single-source executable, so these are separate link units. No conflict. - Teardown on the fabricated session.
setConnected(false)→m_session.reset()→~IcomSession()→stop(). Every branch inIcomSession::stop()(IcomSession.cpp:125-175) is null-guarded —if (m_control && m_control->isReady())for the deauth,if (*s)for each stream,if (*t)for each timer — andstart()callsstop()first on a fresh object, so that path is production-exercised. No null deref. - Does marking the HL2 link up newly reach unsafe code?
setTxSlice(Hl2Backend.cpp:3491) writes TX registers viasetTxFrequencywith no Metis client present. I did not audit that path line by line, because these same promote assertions were passing on main before #6199 — which meanssetTxSliceon a never-started Hl2Backend was already exercised by this exact test. The PR restores a precondition the test previously had implicitly. - Leaking fixture state. Both hunks restore
setConnected(..., false)at the end, so no later section inherits a bogus-connected backend. The dispatch isQt::DirectConnection(RadioModel.cpp:1928-1929), sosetMicLevel(70)completes synchronously with no event-loop turn in between — nothing can observe the fabricated session or fire a timer inside the window. - Does the first assertion still hold after the rebuild?
check(!micGainReported(*rebuilt))is evaluated beforerebuildBackendForTest, on the backend it names, so the new block does not weaken the construction-push gate the section was originally written to pin.
Limits, stated plainly: everything above is reasoned from source in the PR head checkout — I have no build and ran nothing. And the green checks on this head prove compilation, not passage: all three modified tests are, as the body says, outside every focused set, so no CI lane here executed any of them. The only evidence they pass is your local Linux/Qt 6.12 run plus the still-unchecked "main Full Suite on the squash commit" box.
Recommendation
Approve with nits. The diagnosis is correct, the scope is exactly three test files, the preconditions moved rather than the assertions, and each replacement genuinely fails when the dispatch it pins is removed — I checked all three against the dispatch sites rather than taking the negative-control box on faith. The if (live) hole at tests/icom_family_test.cpp:1024 is the one thing worth changing, since an unchecked dynamic_cast behind a check that cannot prove the cast's premise is precisely the silent-pass shape this PR is cleaning up. Since the PR is already closed, the concrete next step is a one-line follow-up adding check(live != nullptr, ...), ideally alongside confirmation that the main Full Suite came back with only #6418 outstanding. Thanks for picking up the full-suite breakage promptly and for scoping out #6418 instead of bundling it.
🤖 aethersdr-agent · cost: $10.4498 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (343 finding(s))
Automated static scan by Cisco DefenseClaw CodeGuard on the changed files. Advisory — some may be false positives; the review above verifies them. 🤖 aethersdr-agent · cost: $10.5780 · model: claude-opus-5 |
Summary
#6199 moved the remaining slice and transmit controls onto typed
IRadioBackendrequests. Those requests are dispatched only to a connected radio with an open TX session. Three tests outside every focused set still described the retired route, and they fail onmain's unfiltered Full Suite atd3ab349f3(run 38302350478):fm_tone_presentation_contractgreppedRadioModel.cppfor the removed&SliceModel::fmDtcsCommandIssuedforward andm_backend->setSliceFmDtcs(. It now checks the current route:SliceModelpublishes the normalizedDtcsSettingrequest, and the Icom adapter'sField::FmDtcscase applies it withsetSliceFmDtcs(.icom_family_test: the operator's mic edit ran against a backend whose connect attempt to TEST-NET never completes, so TX admission stays closed exactly as it does in production untilonConnected(). The edit now runs on a backend rebuilt through the existing socket-freeRadioModel::rebuildBackendForTest(), with its link marked up and an unstartedIcomSession(no streams or sockets).hl2_tci_signaling_test: the HL2 link is marked up before the TCI client promotes a slice, as it is when a TCI client talks to a live radio. No Metis client is started.Every assertion message and expected value is unchanged; only the preconditions moved. This does not touch
radio_setup_headphone_output_gate_test, which has been failing since #6413 (#6418).Closes #6420.
Constitution principle honored
Principle XI: the fix is demonstrated by CI re-running on
main. These tests are the Full Suite's evidence for the #6199 route, and each one now fails when the backend dispatch it pins is removed.Test plan
Field::MicGainreturningUnsupportedfailsicom_family_test's mic assertion; HL2Field::TxSlicereturningUnsupportedfails bothhl2_tci_signaling_testpromote assertions; removing the IcomsetSliceFmDtcs(dispatch fails the contract. Each restored passes.check_test_registration.py, whitespace.mainFull Suite on the squash commit (expected: only the pre-existing [full-suite] test suite failing on main (fp 45383ccd1a1a) #6418 failure remains).🤖 Generated with Claude Code