Tests for the guards whose removal fails closed - #199
Merged
Conversation
The second half of the mutation-testing findings. Where the previous branch covered guards that let something happen, these let something STOP happening -- a limiter that refuses more than it should, a discovery pass that identifies less than it could. Less urgent, and the same defect: a branch validated by nothing. Each assertion below was proven by breaking the code it guards and watching it fail. RATE LIMITER. The limiter has two ways to say yes after a quiet period -- the burst refilling, and a separate "one through per minInterval" allowance -- and the test asserted a single Ok, which cannot tell them apart. Deleting the refill left it green. It now asserts the whole burst is back and that the one after it is refused, so the refill restored the allowance rather than removing the limit. This is the limiter the DRM mode switch charges against. DISCOVERY, six guards, none of them previously reached by a fixture: probesAgree() compares serial AND model; only the serial was ever varied, so a device naming a different model on the second ask would have been called consistent. The fake driver gains modelOnRepeat, mirroring serialOnRepeat. The consistency veto sits after the confidence threshold, and the existing test scored 100 -- halved to 50, already below the threshold of 80. The threshold did the blocking and the veto was never exercised. A score of 200 halves to 100 and puts the veto on its own; the same candidate with the veto disabled IS selected, which is what proves which check did the work. mergeDuplicateSerials() matches on driver id as well as serial. Nothing had two different drivers reporting one serial, so dropping the id comparison silently deleted one of two genuinely different protocol candidates -- the disambiguation the user needs. The same function skips candidates with an EMPTY serial, because nothing can be matched on and two silent units are two units. Nothing had two of them. Removing the guard folds two real inverters into one and the owner configures half a bus. hasSweepableAddress() requires a default value, and numericOption() refuses a default outside the option's own declared bounds -- whose comment names the failure exactly: it "would be probed, reported, offered by the wizard, and then refused by the PATCH gate: a dead end". Both defend against a malformed driver DECLARATION rather than a misbehaving device, which is why no fixture built to describe a device ever produced one. MAXTALK. No test corrupted the "|64:" payload marker, so a frame with a garbled or shifted marker would be parsed from the wrong offset -- and everything after it reads as code=value pairs, so the failure is plausible readings from the wrong place rather than no reading. And the lowercase branch of hexValue() was never executed: our encoder emits uppercase, but what our encoder emits is not evidence about what a device emits, and the vendor is gone. SOLARMAX. The probe test asserted confidenceScore > 0. Discovery RANKS drivers by that number -- 100 read back a serial, 95 a SunSpec identity block, 40 an ambiguous match, 30 the mock -- so it is a position on a scale, not a flag, and > 0 accepted any of them. And poll()'s last two lines, which copy identity and capabilities into the DeviceState, could both be deleted with the suite green: every test read them off the DRIVER, never off the state that REST, MQTT, Prometheus and the register map all publish from. CONFIG BACKUP. The oversized-file fixture was a wall of 'x', which deserializeJson rejects on its own -- so the size ceiling could be deleted and the test could not tell "too big" from "not JSON". It is now oversized VALID JSON, and asserts the message says it was the size. 1032 native cases pass; check_layering.sh passes. Tests only, plus two fields on the discovery fake.
Review caught four new assertions using TEST_ASSERT_EQUAL_UINT32 on a size_t. Unity casts both operands to a 32-bit type, so these narrow on the 64-bit test host -- harmless for the values involved, and the wrong macro regardless. Corrected where the file already has an idiom to match: test_discovery uses TEST_ASSERT_EQUAL_size_t in 23 places, test_maxtalk in every count assertion (and UINT32 only for the 16-bit reading VALUES, which is right). Left alone in test_mqtt and test_drm, where UINT32 on a .size() is what the surrounding assertions already do -- changing those is a separate decision about those files, not part of this branch.
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.
Last of four PRs from the codebase review. Where #197 covered guards that let something happen, these let something stop happening — a limiter that refuses more than it should, a discovery pass that identifies less than it could. Less urgent, same defect: a branch validated by nothing.
Each assertion was proven by breaking the code it guards and watching it fail. Tests only, plus two fields on the discovery fake.
Rate limiter
The limiter has two ways to say yes after a quiet period — the burst refilling, and a separate "one through per
minInterval" allowance. The test asserted a singleOk, which cannot tell them apart, so deleting the refill left it green: a burst of three that behaved like a burst of one, silently.It now asserts the whole burst is back and that the one after it is refused. This is the limiter the DRM mode switch charges against.
Discovery — six guards, none previously reached by a fixture
probesAgree()model checkhasSweepableAddress()defaultnumericOption()boundsThe veto test is the interesting one: a score of 200 halves to 100 and clears the threshold, putting the veto on its own — and the same candidate with the veto disabled is selected, which is what proves which check did the work.
The last two defend against a malformed driver declaration rather than a misbehaving device, which is exactly why no fixture built to describe a device ever produced one.
MaxTalk
No test corrupted the
|64:payload marker, so a garbled or shifted marker would be parsed from the wrong offset — and everything after it reads ascode=valuepairs, so the failure is plausible readings from the wrong place, not a missing reading.The lowercase branch of
hexValue()was never executed. Our encoder emits uppercase, but what our encoder emits is not evidence about what a device emits, and the vendor is gone.SolarMax
TEST_ASSERT_TRUE(r.confidenceScore > 0)— but discovery ranks drivers by that number (100 read back a serial, 95 a SunSpec identity block, 40 an ambiguous match, 30 the mock). It is a position on a scale, and> 0accepted any of them.And
poll()'s last two lines — copying identity and capabilities into theDeviceState— could both be deleted with the suite green: every test read them off the driver, never off the state that REST, MQTT, Prometheus and the register map all publish from.Config backup
The oversized-file fixture was a wall of
x, whichdeserializeJsonrejects on its own — so the size ceiling could be deleted and the test could not tell "too big" from "not JSON". It is now oversized valid JSON, and asserts the message says it was the size.Verification
1032 native cases pass ·
check_layering.shpasses.