From 961d32cac76761b63427f5b2e59c22d5ecd92b03 Mon Sep 17 00:00:00 2001 From: Tim de Bruijn Date: Sat, 29 Aug 2026 00:17:33 +0200 Subject: [PATCH 1/2] Tests for the guards whose removal fails closed 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. --- test/test_commands/test_main.cpp | 16 ++- test/test_config_backup/test_main.cpp | 15 +++ test/test_discovery/test_main.cpp | 146 +++++++++++++++++++++++++- test/test_maxtalk/test_main.cpp | 65 ++++++++++++ test/test_solarmax/test_main.cpp | 35 +++++- 5 files changed, 273 insertions(+), 4 deletions(-) diff --git a/test/test_commands/test_main.cpp b/test/test_commands/test_main.cpp index aca2cee..2cb850b 100644 --- a/test/test_commands/test_main.cpp +++ b/test/test_commands/test_main.cpp @@ -262,7 +262,21 @@ static void test_the_allowance_refills_after_a_quiet_period() { d.dispatch(cmd(InverterCommandType::SetActivePowerLimitPercent, 50.0), driver); } g_now += 2000; - TEST_ASSERT_EQUAL(CommandResult::Ok, + + // THE WHOLE BURST must be back, not just one command. The limiter has two ways to say + // yes after a quiet period -- the burst refilling, and the separate "one through per + // minInterval" allowance -- and asserting a single Ok cannot tell them apart. It passed + // with the refill deleted, which would have shipped a burst of three that behaved like a + // burst of one, silently, on the path that asserts a DRM mode. + for (int i = 0; i < 3; ++i) { + TEST_ASSERT_EQUAL_MESSAGE( + CommandResult::Ok, + d.dispatch(cmd(InverterCommandType::SetActivePowerLimitPercent, 50.0), driver).result, + "the refilled allowance must be the full burst"); + } + // And the fourth is refused again, so the refill restored the allowance rather than + // removing the limit. + TEST_ASSERT_EQUAL(CommandResult::RateLimited, d.dispatch(cmd(InverterCommandType::SetActivePowerLimitPercent, 50.0), driver) .result); } diff --git a/test/test_config_backup/test_main.cpp b/test/test_config_backup/test_main.cpp index ea7b182..a772d3d 100644 --- a/test/test_config_backup/test_main.cpp +++ b/test/test_config_backup/test_main.cpp @@ -319,6 +319,21 @@ static void test_a_configuration_that_fails_validation_is_refused() { static void test_an_oversized_file_is_refused_before_parsing() { BackupContents contents; std::string detail; + + // VALID JSON, and too big. The fixture used to be a wall of 'x', which deserializeJson + // rejects on its own -- so deleting the size ceiling entirely left this green and the + // test could not tell "refused for being oversized" from "refused for not being JSON". + // The ceiling exists to stop a large well-formed document reaching the parser at all, on + // a device where that memory is not available to lose. + std::string big = R"({"format_version":1,"configuration":{"note":")"; + big.append(kMaxBackupBytes, 'a'); + big += R"("}})"; + TEST_ASSERT_TRUE_MESSAGE(big.size() > kMaxBackupBytes, "the fixture must exceed the ceiling"); + TEST_ASSERT_EQUAL(BackupResult::NotJson, parseConfigBackup(big, contents, detail)); + TEST_ASSERT_TRUE_MESSAGE(detail.find("larger") != std::string::npos, + "and must say it was the SIZE, not the syntax"); + + // The original case still holds: not-JSON is also refused, for its own reason. TEST_ASSERT_EQUAL(BackupResult::NotJson, parseConfigBackup(std::string(kMaxBackupBytes + 1, 'x'), contents, detail)); } diff --git a/test/test_discovery/test_main.cpp b/test/test_discovery/test_main.cpp index 40f2f38..8064af3 100644 --- a/test/test_discovery/test_main.cpp +++ b/test/test_discovery/test_main.cpp @@ -31,6 +31,9 @@ class FakeDriver : public InverterDriver { std::string model = "Model-1"; /// Second and later probes report this serial instead, simulating an unstable match. std::string serialOnRepeat; + /// The same, for the model. probesAgree() compares serial AND model, and only the + /// serial half was ever varied -- so dropping the model comparison changed nothing. + std::string modelOnRepeat; bool writeAttempted = false; /// Non-zero: begin() reconfigures the line to this, as every real driver does. uint32_t begunAtBaud = 0; @@ -71,7 +74,9 @@ class FakeDriver : public InverterDriver { r.responded = script_->responded; r.checksumValid = script_->checksumValid; r.confidenceScore = script_->score; - r.detectedModel = script_->model; + r.detectedModel = (probeCount_ > 1 && !script_->modelOnRepeat.empty()) + ? script_->modelOnRepeat + : script_->model; r.serialNumber = (probeCount_ > 1 && !script_->serialOnRepeat.empty()) ? script_->serialOnRepeat : script_->serial; @@ -127,6 +132,10 @@ struct AddressBus { std::vector probed; /// Same serial at every address, for the one-device-two-addresses case. bool sharedSerial = false; + /// No serial at all, which several protocols genuinely cannot report. Two silent-serial + /// units are two units, and mergeDuplicateSerials() has a guard saying so that no fixture + /// ever reached. + bool emptySerial = false; }; class AddressedDriver : public InverterDriver { @@ -151,7 +160,9 @@ class AddressedDriver : public InverterDriver { r.checksumValid = true; r.confidenceScore = 95; r.detectedModel = "MIC-TL-X"; - r.serialNumber = bus_->sharedSerial ? "SHARED" : "SER-" + address_; + r.serialNumber = bus_->emptySerial ? std::string() + : bus_->sharedSerial ? std::string("SHARED") + : "SER-" + address_; return r; } @@ -576,6 +587,131 @@ static void test_threshold_is_configurable() { TEST_ASSERT_TRUE(e.run(DiscoveryMode::Quick, c).autoSelected); } +// probesAgree() compares responded, serial AND model. Only the serial half was ever varied by +// a fixture, so deleting the model comparison left the suite green: a device naming a different +// model on the second ask would have been called consistent and auto-selected. +// Two descriptor guards that nothing reached. Both defend the wizard against a MALFORMED +// driver declaration rather than against a misbehaving device, so no fixture built to describe +// a device ever produced one -- which is precisely why they went unverified. + +// A numeric address option with no default cannot be swept: probesFor() puts the driver's own +// default first and always includes it, so an empty one would sweep an address the driver never +// declared. Removing the defaultValue requirement left the suite green. +// The mirror image of the one-device-two-addresses case. mergeDuplicateSerials() folds +// candidates that share a serial, and skips any candidate whose serial is EMPTY -- because +// nothing can be matched on, so two silent units are two units. No fixture ever had two +// responding candidates without a serial, so deleting that guard went unnoticed: it would have +// collapsed two genuinely distinct inverters into one, and the owner would configure half a bus. +static void test_two_devices_without_serial_numbers_are_not_folded_into_one() { + DriverRegistry reg; + MockTransport t; + AddressBus bus; + bus.occupied = {"1", "2"}; + bus.emptySerial = true; + addAddressedDriver(reg, addressedDesc("silent_serial", "1"), &bus); + + DiscoveryEngine e(reg, t); + const auto out = e.run(DiscoveryMode::Extended); + + TEST_ASSERT_EQUAL_UINT32(2, out.candidates.size()); +} + +static void test_an_address_option_without_a_default_is_not_sweepable() { + TEST_ASSERT_TRUE(addressedDesc("ok", "1").hasSweepableAddress()); + TEST_ASSERT_FALSE_MESSAGE(addressedDesc("no-default", "").hasSweepableAddress(), + "a numeric address option with no default cannot anchor a sweep"); +} + +// A default outside the option's OWN declared bounds. The guard's comment names the failure +// exactly: such a value "would be probed, reported, offered by the wizard, and then refused by +// the PATCH gate: a dead end". No driver in the suite declared one, so the bounds check was +// validated by nothing. +static void test_a_default_outside_its_own_bounds_is_refused_rather_than_offered() { + const DriverDescriptor inRange = addressedDesc("sane", "5", 1, 8); + long out = 0; + TEST_ASSERT_TRUE(inRange.numericOption({}, "unit_id", out)); + TEST_ASSERT_EQUAL_INT32(5, out); + + // 10 is SolaX's real default and outside a 1..8 declaration -- the shape this guards. + const DriverDescriptor contradictory = addressedDesc("contradictory", "10", 1, 8); + out = 0; + TEST_ASSERT_FALSE_MESSAGE(contradictory.numericOption({}, "unit_id", out), + "a default outside its own bounds is a dead end, not an address"); + TEST_ASSERT_EQUAL_INT32(0, out); // and nothing is written on refusal +} + +static void test_a_device_that_names_a_different_model_on_the_second_ask_is_inconsistent() { + DriverRegistry reg; + MockTransport t; + FakeDriver::Script s; + s.score = 100; + s.serial = "SER-1"; // the serial AGREES; only the model moves + s.model = "Model-A"; + s.modelOnRepeat = "Model-B"; + addDriver(reg, desc("shifty", 10), &s); + + DiscoveryEngine e(reg, t); + const auto out = e.run(DiscoveryMode::Quick); + + TEST_ASSERT_FALSE(out.candidates[0].consistent); + TEST_ASSERT_EQUAL_INT(50, out.candidates[0].probe.confidenceScore); + TEST_ASSERT_FALSE(out.autoSelected); +} + +// The consistency veto sits AFTER the confidence threshold, and the existing test scores 100 -- +// halved to 50, already below the threshold of 80. So the threshold did the blocking and the +// veto itself was never exercised: removing it changed nothing. A score that survives halving +// is what puts the veto on its own. +static void test_the_consistency_veto_blocks_on_its_own_not_only_via_the_threshold() { + DriverRegistry reg; + MockTransport t; + FakeDriver::Script s; + s.score = 200; // halves to 100, comfortably above minConfidence + s.serial = "SER-1"; + s.serialOnRepeat = "SER-2"; + addDriver(reg, desc("flaky", 10), &s); + + DiscoveryEngine e(reg, t); + const auto out = e.run(DiscoveryMode::Quick); + + TEST_ASSERT_EQUAL_INT(100, out.candidates[0].probe.confidenceScore); + TEST_ASSERT_FALSE_MESSAGE(out.autoSelected, "an inconsistent probe must not be selected"); + TEST_ASSERT_TRUE(out.reason.find("disagreed") != std::string::npos); + + // The same candidate WITH the veto disabled is selected -- which is what proves the veto + // did the blocking, and not the threshold or the margin. + DriverRegistry reg2; + MockTransport t2; + FakeDriver::Script s2 = s; + addDriver(reg2, desc("flaky", 10), &s2); + DiscoveryConfig permissive; + permissive.requireConsistentProbes = false; + DiscoveryEngine e2(reg2, t2); + TEST_ASSERT_TRUE(e2.run(DiscoveryMode::Quick, permissive).autoSelected); +} + +// mergeDuplicateSerials() folds two candidates together only when the DRIVER ID matches as well +// as the serial. No fixture ever had two different drivers reporting the same serial, so +// dropping the id comparison went unnoticed -- and it would have silently deleted one of two +// genuinely different protocol candidates, which is exactly the disambiguation the user needs. +static void test_two_different_drivers_reporting_one_serial_stay_two_candidates() { + DriverRegistry reg; + MockTransport t; + FakeDriver::Script a; + FakeDriver::Script b; + a.score = 90; + b.score = 85; + a.serial = "SAME-SERIAL"; + b.serial = "SAME-SERIAL"; + addDriver(reg, desc("driver-a", 10), &a); + addDriver(reg, desc("driver-b", 20), &b); + + DiscoveryEngine e(reg, t); + const auto out = e.run(DiscoveryMode::Quick); + + TEST_ASSERT_EQUAL_UINT32(2, out.candidates.size()); +} + static void test_inconsistent_probes_halve_the_score_and_block_selection() { // A device that identifies as something different on the second ask was never identified. DriverRegistry reg; @@ -882,6 +1018,12 @@ int main(int, char**) { RUN_TEST(test_a_score_below_the_threshold_is_never_auto_selected); RUN_TEST(test_threshold_is_configurable); RUN_TEST(test_inconsistent_probes_halve_the_score_and_block_selection); + RUN_TEST(test_two_devices_without_serial_numbers_are_not_folded_into_one); + RUN_TEST(test_an_address_option_without_a_default_is_not_sweepable); + RUN_TEST(test_a_default_outside_its_own_bounds_is_refused_rather_than_offered); + RUN_TEST(test_a_device_that_names_a_different_model_on_the_second_ask_is_inconsistent); + RUN_TEST(test_the_consistency_veto_blocks_on_its_own_not_only_via_the_threshold); + RUN_TEST(test_two_different_drivers_reporting_one_serial_stay_two_candidates); RUN_TEST(test_consistent_probes_are_recorded_as_evidence); RUN_TEST(test_a_failed_checksum_blocks_selection); RUN_TEST(test_a_silent_bus_yields_no_candidates_and_a_useful_reason); diff --git a/test/test_maxtalk/test_main.cpp b/test/test_maxtalk/test_main.cpp index 2b6a0bc..7cd219f 100644 --- a/test/test_maxtalk/test_main.cpp +++ b/test/test_maxtalk/test_main.cpp @@ -230,6 +230,69 @@ static void test_frame_length_finds_the_boundary_in_a_longer_buffer() { TEST_ASSERT_EQUAL_size_t(n, maxtalk::frameLength(buf, n + 7)); } +/// Builds an otherwise-perfect reply -- correct declared length, correct checksum -- around a +/// caller-chosen marker and payload. Both callers below need to vary exactly one thing and keep +/// every other check satisfied, or the parser refuses the frame before reaching what is under +/// test. That is the failure this suite has already been bitten by once. +static size_t buildReply(const char* marker, const char* payload, char* frame) { + const size_t total = 9 + (std::strlen(marker) + std::strlen(payload) + 1) + 5; + int w = 0; + frame[w++] = '{'; + frame[w++] = '0'; frame[w++] = '5'; frame[w++] = ';'; + frame[w++] = 'F'; frame[w++] = 'B'; frame[w++] = ';'; + frame[w++] = static_cast(total / 16 < 10 ? '0' + total / 16 : 'A' + total / 16 - 10); + frame[w++] = static_cast(total % 16 < 10 ? '0' + total % 16 : 'A' + total % 16 - 10); + std::memcpy(frame + w, marker, std::strlen(marker)); + w += static_cast(std::strlen(marker)); + std::memcpy(frame + w, payload, std::strlen(payload)); + w += static_cast(std::strlen(payload)); + frame[w++] = '|'; + const uint16_t sum = maxtalk::checksum(frame + 1, static_cast(w) - 1); + for (int i = 0; i < 4; ++i) { + const uint8_t nib = (sum >> (12 - 4 * i)) & 0xF; + frame[w++] = static_cast(nib < 10 ? '0' + nib : 'A' + nib - 10); + } + frame[w++] = '}'; + return static_cast(w); +} + +// No test ever corrupted the "|64:" marker, so deleting the memcmp that checks it left the +// suite green. A frame whose marker is garbled or shifted by a byte would then be parsed from +// the wrong offset -- and everything after it is read as code=value pairs, so the failure is +// not "no reading" but plausible readings taken from the wrong place in the frame. +static void test_a_frame_whose_payload_marker_is_wrong_is_refused() { + const char* markers[] = {"|65:", "|64;", "@64:", "|640", "::::"}; + for (const char* m : markers) { + char frame[maxtalk::kMaxFrame]; + const size_t n = buildReply(m, "CAC=1F3E", frame); + + maxtalk::Reading readings[4]; + size_t count = 0; + TEST_ASSERT_EQUAL_MESSAGE(maxtalk::ParseResult::Malformed, + maxtalk::parseReply(frame, n, 0x05, readings, 4, count), m); + } +} + +// The codec writes uppercase, so every fixture in this suite is uppercase and the lowercase +// branch of hexValue() was never executed -- deleting it changed nothing. The vendor is gone +// and there is no second implementation to compare against, so "our own encoder never emits +// it" is not evidence about what a device emits. The branch exists; this is what it claims. +static void test_a_lowercase_hex_value_decodes_the_same_as_uppercase() { + char upper[maxtalk::kMaxFrame]; + char lower[maxtalk::kMaxFrame]; + const size_t nu = buildReply("|64:", "CAC=1F3E", upper); + const size_t nl = buildReply("|64:", "CAC=1f3e", lower); + + maxtalk::Reading ru[4], rl[4]; + size_t cu = 0, cl = 0; + TEST_ASSERT_EQUAL(maxtalk::ParseResult::Ok, maxtalk::parseReply(upper, nu, 0x05, ru, 4, cu)); + TEST_ASSERT_EQUAL(maxtalk::ParseResult::Ok, maxtalk::parseReply(lower, nl, 0x05, rl, 4, cl)); + TEST_ASSERT_EQUAL_UINT32(1, cu); + TEST_ASSERT_EQUAL_UINT32(1, cl); + TEST_ASSERT_EQUAL_UINT32(0x1F3E, ru[0].value); + TEST_ASSERT_EQUAL_UINT32(ru[0].value, rl[0].value); +} + static void test_a_malformed_payload_is_refused_rather_than_half_decoded() { // The frame must be otherwise PERFECT -- correct length, correct checksum -- or the parser // rejects it earlier and the payload logic is never reached. An earlier version of this test @@ -338,6 +401,8 @@ int main() { RUN_TEST(test_an_unterminated_frame_asks_for_more_bytes); RUN_TEST(test_frame_length_finds_the_boundary_in_a_longer_buffer); RUN_TEST(test_a_malformed_payload_is_refused_rather_than_half_decoded); + RUN_TEST(test_a_frame_whose_payload_marker_is_wrong_is_refused); + RUN_TEST(test_a_lowercase_hex_value_decodes_the_same_as_uppercase); RUN_TEST(test_running_out_of_room_keeps_what_was_already_decoded); RUN_TEST(test_a_request_that_does_not_fit_is_refused_rather_than_truncated); RUN_TEST(test_a_request_is_refused_for_codes_that_cannot_exist); diff --git a/test/test_solarmax/test_main.cpp b/test/test_solarmax/test_main.cpp index 99fe407..9e5539e 100644 --- a/test/test_solarmax/test_main.cpp +++ b/test/test_solarmax/test_main.cpp @@ -235,6 +235,32 @@ static void test_an_unusable_reply_leaves_the_state_completely_untouched() { TEST_ASSERT_EQUAL_STRING("untouched", s.statusText.c_str()); } +// Every other test in this suite reads capabilities and identity off the DRIVER (d.capabilities(), +// d.identity()). Nothing read them off the polled DeviceState -- which is the copy REST, MQTT, +// Prometheus and the Modbus register map all publish from. Deleting both assignments at the end +// of poll() therefore left the whole suite green, while a bridge would have reported a device +// with no manufacturer, no protocol name and no capabilities at all. +static void test_a_poll_publishes_the_identity_and_capabilities_into_the_state() { + MockTransport t; + solarmax::SolarmaxDriver d(t, solarmax::SolarmaxOptions{0x05}); + d.begin(t); + respondWith(t, 0x05, "PAC=1F4;UL1=8FC;IL1=64;TNF=1388;KDY=1F;KT0=3E8"); + + DeviceState s; + TEST_ASSERT_EQUAL(PollResult::Ok, d.poll(s)); + + TEST_ASSERT_EQUAL_STRING("SolarMax", s.identity.manufacturer.c_str()); + TEST_ASSERT_EQUAL_STRING("MaxTalk RS485", s.identity.protocolName.c_str()); + TEST_ASSERT_EQUAL_STRING(d.identity().driverId.c_str(), s.identity.driverId.c_str()); + + TEST_ASSERT_TRUE(s.capabilities.has(InverterCapability::ReadAcPower)); + TEST_ASSERT_EQUAL_UINT8(d.capabilities().phaseCount, s.capabilities.phaseCount); + TEST_ASSERT_EQUAL_UINT8(d.capabilities().mpptCount, s.capabilities.mpptCount); + // Read-only is what every output keys its control surface off, so it has to survive the + // copy rather than be inferred from the driver id downstream. + TEST_ASSERT_TRUE(s.capabilities.isReadOnly()); +} + static void test_the_probe_identifies_without_writing_anything_but_a_query() { MockTransport t; solarmax::SolarmaxDriver d(t, solarmax::SolarmaxOptions{0x05}); @@ -245,7 +271,13 @@ static void test_the_probe_identifies_without_writing_anything_but_a_query() { TEST_ASSERT_TRUE(r.responded); TEST_ASSERT_TRUE(r.checksumValid); TEST_ASSERT_EQUAL_STRING("SolarMax", r.detectedManufacturer.c_str()); - TEST_ASSERT_TRUE(r.confidenceScore > 0); + // The exact score, not merely "some confidence". Discovery ranks candidate drivers against + // each other by this number, so it is a position on a scale rather than a flag: 100 is a + // driver that read back a serial number (EverSolar, SolaX), 95 a SunSpec device that + // answered its identity block, 40 an ambiguous SunSpec match, 30 the mock. 70 says "the + // protocol answered and named a type, but this family has no serial to confirm it with". + // Asserting > 0 accepted any of those, and the ranking is the whole point. + TEST_ASSERT_EQUAL_INT(70, r.confidenceScore); // Hex on the wire, hex in the label: "type 0x64", not the decimal 100. TEST_ASSERT_EQUAL_STRING("type 0x64", r.detectedModel.c_str()); @@ -315,6 +347,7 @@ int main() { RUN_TEST(test_a_reply_with_no_mapped_codes_is_not_a_successful_poll); RUN_TEST(test_an_unusable_reply_leaves_the_state_completely_untouched); RUN_TEST(test_the_probe_identifies_without_writing_anything_but_a_query); + RUN_TEST(test_a_poll_publishes_the_identity_and_capabilities_into_the_state); RUN_TEST(test_a_silent_bus_probes_as_no_device_rather_than_a_broken_one); RUN_TEST(test_another_devices_frame_during_a_probe_is_recorded_as_traffic); RUN_TEST(test_the_driver_refuses_every_write); From f8630adcc63050964ae56d07d82e9b38749675ed Mon Sep 17 00:00:00 2001 From: Tim de Bruijn Date: Sat, 29 Aug 2026 00:36:35 +0200 Subject: [PATCH 2/2] Assert sizes at the width they actually are 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. --- test/test_discovery/test_main.cpp | 4 ++-- test/test_maxtalk/test_main.cpp | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/test/test_discovery/test_main.cpp b/test/test_discovery/test_main.cpp index 8064af3..441c7b5 100644 --- a/test/test_discovery/test_main.cpp +++ b/test/test_discovery/test_main.cpp @@ -613,7 +613,7 @@ static void test_two_devices_without_serial_numbers_are_not_folded_into_one() { DiscoveryEngine e(reg, t); const auto out = e.run(DiscoveryMode::Extended); - TEST_ASSERT_EQUAL_UINT32(2, out.candidates.size()); + TEST_ASSERT_EQUAL_size_t(2, out.candidates.size()); } static void test_an_address_option_without_a_default_is_not_sweepable() { @@ -709,7 +709,7 @@ static void test_two_different_drivers_reporting_one_serial_stay_two_candidates( DiscoveryEngine e(reg, t); const auto out = e.run(DiscoveryMode::Quick); - TEST_ASSERT_EQUAL_UINT32(2, out.candidates.size()); + TEST_ASSERT_EQUAL_size_t(2, out.candidates.size()); } static void test_inconsistent_probes_halve_the_score_and_block_selection() { diff --git a/test/test_maxtalk/test_main.cpp b/test/test_maxtalk/test_main.cpp index 7cd219f..ea6f941 100644 --- a/test/test_maxtalk/test_main.cpp +++ b/test/test_maxtalk/test_main.cpp @@ -287,8 +287,8 @@ static void test_a_lowercase_hex_value_decodes_the_same_as_uppercase() { size_t cu = 0, cl = 0; TEST_ASSERT_EQUAL(maxtalk::ParseResult::Ok, maxtalk::parseReply(upper, nu, 0x05, ru, 4, cu)); TEST_ASSERT_EQUAL(maxtalk::ParseResult::Ok, maxtalk::parseReply(lower, nl, 0x05, rl, 4, cl)); - TEST_ASSERT_EQUAL_UINT32(1, cu); - TEST_ASSERT_EQUAL_UINT32(1, cl); + TEST_ASSERT_EQUAL_size_t(1, cu); + TEST_ASSERT_EQUAL_size_t(1, cl); TEST_ASSERT_EQUAL_UINT32(0x1F3E, ru[0].value); TEST_ASSERT_EQUAL_UINT32(ru[0].value, rl[0].value); }