Tests for the guards whose removal fails open - #197
Open
Timdebruijn wants to merge 2 commits into
Open
Conversation
Mutation testing across thirteen suites (114 mutations) found guards that no test
protects. These are the ones where broken code does something rather than nothing.
RELAYS. patternFor() refuses mode "none", and that guard is load-bearing, not
defensive: isValidRole("none") returns true, so without it "none" reaches the match
loop. applyDrmMode() pads the roles list with "none" up to the relay count, and REST
validates only that the mode string is 1..16 characters -- so POST /api/v1/drm/set
?mode=none would energise every unassigned relay on a 6CH wired to DRM inputs. The
test that claimed this passed {"drm0"}, where the loop finds no match and returns
false whether the guard is there or not. It now passes a roles list that contains
"none". Also covers optionsFor()'s isValidRole filter, which no fixture reached.
WRITES. writeSingleRegister had no tests at all -- the name did not appear anywhere
under test/ -- while modbus_profile and sunspec both call it. Replacing its echo
comparison with `accepted = true` left the suite green, so a device echoing a
different address or value would have been recorded as Ok: a setpoint that never
arrived, confirmed as delivered. Five cases now: a good echo, a wrong address, a
wrong value, an exception with its code, and silence.
Two more codec guards had the same shape. parseReadResponse's odd-byte-count check
(a malformed frame would decode as one good register) and parseWriteResponse's
function-code echo on the SUCCESS path -- covered only via the exception path, which
is a different branch, as the mutation run confirmed by leaving that test green while
the new one failed.
RESPONSE SIZE. json_limits::finish() is the single choke point all 44 payload builders
share. Deleting its `needed > maxBytes` check left three "still fits in the response"
tests green, because their fixtures are naturally well under the cap; only
test_oversized_response_is_refused, which forces maxBytes=50, actually covered it.
Each of the three now also asserts the builder refuses at one byte under the measured
size -- self-calibrating, so it cannot go vacuous as payloads grow. Four tests catch
that mutation now instead of one.
CAPABILITIES. test_a_writable_driver_flips_the_read_only_register asserted
TEST_ASSERT_TRUE(anyWriteBit), an OR across four registers. Publishing the READ bitmap
into the write registers kept the OR true, so the one test that says which channels are
writable agreed with the wrong bitmap. It now compares the reconstructed 64-bit value
against the driver's own declaration, and asserts the fixture's read and write sets
differ so the comparison can tell them apart.
Every assertion here was proven by breaking the code it guards and watching it fail,
then restoring. 1030 native cases pass; check_layering.sh passes.
Review caught an overclaim in three of the comments this branch added: finish() is shared by 24 call sites, not 44. The 44 came from a grep that counted its definition, its own comments and unrelated `finish(` lines along with the calls -- the same shape of mistake these tests exist to catch, made while writing them. The argument is unchanged, and the number was never load-bearing. But a comment that overstates its evidence is the thing that makes the next reader trust the next number, and this file has been bitten by exactly that before. Worth recording separately, because it is a real gap rather than a wording fix: finish() is not the only serialiser either. home_assistant_discovery.cpp carries a private serialise() that is finish() minus the size check, used for every discovery entity. Addressed on the branch that changes production code, not here.
Timdebruijn
marked this pull request as ready for review
August 28, 2026 23:08
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Second of three PRs from the codebase review. 114 mutations across thirteen suites found guards that no test protects; this covers the ones where broken code does something rather than nothing.
Every assertion below was proven by breaking the code it guards, watching the test fail, and restoring. Tests only — no production code changes.
Relays — the one that moves hardware
patternFor()refuses mode"none", and that guard is load-bearing, not defensive:isValidRole("none")returnstrue, so without it"none"reaches the match loop.The test that claimed to cover this passed
{"drm0"}— no relay carries"none", so the loop returnsfalsewhether the guard exists or not. It now passes a roles list that actually contains one.No live bug: the guard is present and correct. But it could have been refactored away against a green suite, on a 6CH wired to DRM inputs.
Also covers
optionsFor()'sisValidRolefilter, which no fixture reached.Writes — zero coverage on the echo
writeSingleRegisterdid not appear anywhere undertest/, whilemodbus_profileandsunspecboth call it.step.accepted = trueleft the suite green.The header states the contract: a write whose echo is not verified is a request, not a setting. Five cases now — good echo, wrong address, wrong value, exception with code, silence.
Two codec guards had the same shape:
parseReadResponse's odd-byte-count check, andparseWriteResponse's function-code echo on the success path (covered only via the exception path — a different branch, which the mutation run confirmed by leaving that test green while the new one failed).Response size — one choke point, one real test
json_limits::finish()is shared by 44 payload builders. Deleting itsneeded > maxBytescheck left three "still fits" tests green — their fixtures are naturally well under the cap. Onlytest_oversized_response_is_refused(forcingmaxBytes=50) covered it.Each of the three now also asserts refusal at one byte under the measured size — self-calibrating, so it cannot go vacuous as payloads grow. Four tests catch that mutation now instead of one.
Capabilities
TEST_ASSERT_TRUE(anyWriteBit)was an OR across four registers. Publishing the read bitmap into the write registers kept it true. Now compares the reconstructed 64-bit value against the driver's own declaration — plus an assertion that the fixture's read and write sets differ, so the comparison can tell them apart.Verification
pio test -e native: 1030 cases pass.check_layering.shpasses.