diff --git a/test/test_config_backup/test_main.cpp b/test/test_config_backup/test_main.cpp index ea7b182..934c19b 100644 --- a/test/test_config_backup/test_main.cpp +++ b/test/test_config_backup/test_main.cpp @@ -418,6 +418,17 @@ static void test_the_worst_case_preview_still_fits_its_bound() { "the worst-case preview must not exceed kMaxRestorePreviewBytes"); TEST_ASSERT_TRUE_MESSAGE(out.size() < heliograph::rest::kMaxRestorePreviewBytes, "and must fit with room to spare, not exactly"); + + // AND THE BOUND ACTUALLY BITES. The assertion above only says this payload is small + // enough; it says nothing about whether anything would stop a larger one. Mutation + // testing deleted the `needed > maxBytes` check in json_limits::finish() -- the single + // choke point twenty-four payload builders share -- and this test stayed green. Refusing at + // one byte under the measured size is self-calibrating: it cannot go vacuous when the + // payload grows. + std::string refused = "untouched"; + TEST_ASSERT_FALSE(heliograph::rest::buildRestorePreviewPayload(bc, diff, true, true, refused, + out.size() - 1)); + TEST_ASSERT_EQUAL_STRING("untouched", refused.c_str()); } static void test_diff_reports_a_changed_list_position_by_position() { diff --git a/test/test_drm/test_main.cpp b/test/test_drm/test_main.cpp index 164901b..66aa571 100644 --- a/test/test_drm/test_main.cpp +++ b/test/test_drm/test_main.cpp @@ -35,6 +35,15 @@ static void test_options_derive_from_roles() { // Duplicate roles (two relays on one line) appear once. TEST_ASSERT_EQUAL_UINT32(2, optionsFor({"drm0", "drm0"}).size()); + + // A role that is not "none" but is not valid either. Every fixture above feeds + // optionsFor roles it already accepts, so the isValidRole() filter was never + // exercised: garbage in the stored config would have been offered as a mode the + // user could pick, and patternFor() then refuses it -- a dead entry in the select. + const auto filtered = optionsFor({"drm0", "drm9", "DRM0", "", "drm12"}); + TEST_ASSERT_EQUAL_UINT32(2, filtered.size()); + TEST_ASSERT_EQUAL_STRING("normal", filtered[0].c_str()); + TEST_ASSERT_EQUAL_STRING("drm0", filtered[1].c_str()); } static void test_pattern_asserts_exactly_the_role() { @@ -62,8 +71,17 @@ static void test_invalid_modes_are_refused() { TEST_ASSERT_FALSE(patternFor({"drm0"}, "drm5", pattern)); // "custom" is a reported state, never a command. TEST_ASSERT_FALSE(patternFor({"drm0"}, "custom", pattern)); - // "none" is a role, never a mode. + // "none" is a role, never a mode. The roles list MUST contain a "none" for this to + // prove anything: with only {"drm0"} the match loop finds nothing and returns false + // whether the guard survives or not, so the assertion passed against code that had + // lost it. isValidRole("none") is true, so "none" reaches that loop -- and every + // unassigned relay carries exactly that role, because applyDrmMode() pads the list + // with it. Drop the guard and POST /api/v1/drm/set?mode=none energises all of them. TEST_ASSERT_FALSE(patternFor({"drm0"}, "none", pattern)); + TEST_ASSERT_FALSE(patternFor({"none", "drm0", "none"}, "none", pattern)); + for (const bool on : pattern) { + TEST_ASSERT_FALSE(on); + } // "normal" without any roles is meaningless. TEST_ASSERT_FALSE(patternFor({"none"}, "normal", pattern)); // Refusal always leaves a released pattern behind, so a caller that ignores the diff --git a/test/test_modbus/client.cpp b/test/test_modbus/client.cpp index dc1f916..e465576 100644 --- a/test/test_modbus/client.cpp +++ b/test/test_modbus/client.cpp @@ -41,6 +41,18 @@ std::vector exceptionReply(uint8_t unit, uint8_t fn, uint8_t code) { return f; } +/// The echo a device sends back for 0x06: the address and the value it accepted, verbatim. +std::vector writeEcho(uint8_t unit, uint16_t address, uint16_t value) { + std::vector f{unit, + kWriteSingleRegister, + static_cast(address >> 8), + static_cast(address & 0xFF), + static_cast(value >> 8), + static_cast(value & 0xFF)}; + test::appendModbusCrc(f); + return f; +} + } // namespace static void test_a_good_reply_decodes() { @@ -189,6 +201,67 @@ static void test_a_zero_length_read_is_refused() { TEST_ASSERT_TRUE(t.writes.empty()); } +// THE WRITE PATH HAD NO TESTS AT ALL. Every case below is new, and the reason they are worth +// having is the one the header states: a write whose echo is not verified is a request, not a +// setting. Mutation testing found this by replacing the echo comparison with `accepted = true` +// and watching the whole suite stay green -- nothing here called writeSingleRegister, while two +// production drivers do (modbus_profile and sunspec). +static void test_a_write_that_echoes_what_was_sent_is_a_setting() { + MockTransport t; + t.replyWith(writeEcho(kUnit, 0x0410, 750)); + + const auto r = writeSingleRegister(t, kUnit, 0x0410, 750); + + TEST_ASSERT_EQUAL(TransactionStatus::Ok, r.status); +} + +// The two that matter. A device that answers about a DIFFERENT register, or with a different +// value than the one asked for, has not done what was asked however well-formed the frame is. +// Reported as Protocol -- "intact, but not what we asked for" -- and never as Ok, because on a +// control register the difference is between an inverter that is limited and one everybody +// believes is limited. +static void test_a_write_echoing_another_address_is_not_a_setting() { + MockTransport t; + t.replyWith(writeEcho(kUnit, 0x0411, 750)); // neighbouring register + + const auto r = writeSingleRegister(t, kUnit, 0x0410, 750); + + TEST_ASSERT_EQUAL(TransactionStatus::Protocol, r.status); +} + +static void test_a_write_echoing_another_value_is_not_a_setting() { + MockTransport t; + t.replyWith(writeEcho(kUnit, 0x0410, 1000)); // clamped by the device, or stale + + const auto r = writeSingleRegister(t, kUnit, 0x0410, 750); + + TEST_ASSERT_EQUAL(TransactionStatus::Protocol, r.status); +} + +// A refusal is the ORDINARY answer to a control write -- read-only register, value out of +// range, a device wanting an unlock first -- so it carries its code rather than collapsing +// into a protocol error. +static void test_a_refused_write_carries_its_exception_code() { + MockTransport t; + t.replyWith(exceptionReply(kUnit, kWriteSingleRegister, 0x04)); // slave device failure + + const auto r = writeSingleRegister(t, kUnit, 0x0410, 750); + + TEST_ASSERT_EQUAL(TransactionStatus::Exception, r.status); + TEST_ASSERT_EQUAL_UINT8(0x04, r.exceptionCode); +} + +// Silence must never read as "written". This is the failure an unpowered or mis-addressed +// device produces, and it is the one most likely to be met in the field. +static void test_a_write_nobody_answers_is_a_timeout() { + MockTransport t; + t.replyWithSilence(); + + const auto r = writeSingleRegister(t, kUnit, 0x0410, 750); + + TEST_ASSERT_EQUAL(TransactionStatus::Timeout, r.status); +} + void run_modbus_client() { RUN_TEST(test_a_good_reply_decodes); RUN_TEST(test_an_exception_reports_its_code); @@ -200,4 +273,9 @@ void run_modbus_client() { RUN_TEST(test_a_reply_with_too_few_registers_is_refused); RUN_TEST(test_a_reply_with_exactly_the_requested_count_is_ok); RUN_TEST(test_a_zero_length_read_is_refused); + RUN_TEST(test_a_write_that_echoes_what_was_sent_is_a_setting); + RUN_TEST(test_a_write_echoing_another_address_is_not_a_setting); + RUN_TEST(test_a_write_echoing_another_value_is_not_a_setting); + RUN_TEST(test_a_refused_write_carries_its_exception_code); + RUN_TEST(test_a_write_nobody_answers_is_a_timeout); } diff --git a/test/test_modbus/rtu.cpp b/test/test_modbus/rtu.cpp index 4bbdc7d..9fb7d28 100644 --- a/test/test_modbus/rtu.cpp +++ b/test/test_modbus/rtu.cpp @@ -290,6 +290,54 @@ static void test_write_exception_with_a_bad_crc_indicts_the_cable() { parseWriteResponse(frame, 5, 0x01, kWriteSingleRegister, resp)); } +// Both cases below were found by mutation testing: deleting the guard each one exercises left +// the whole suite green, so the branch was validated by nothing. + +// A byte count that is not a whole number of 16-bit registers. Without the guard the count is +// silently halved, the odd trailing byte is dropped, and a malformed frame decodes as if it +// were one good register -- a wrong reading with no error anywhere to say so. +static void test_a_read_reply_with_an_odd_byte_count_is_malformed() { + uint8_t frame[8]; + frame[0] = 0x01; + frame[1] = kReadHoldingRegisters; + frame[2] = 0x03; // three bytes: one register and a half + frame[3] = 0x12; + frame[4] = 0x34; + frame[5] = 0x56; + const uint16_t crc = crc16(frame, 6); + frame[6] = static_cast(crc & 0xFF); + frame[7] = static_cast((crc >> 8) & 0xFF); + + uint16_t regs[4] = {}; + ReadResponse resp; + TEST_ASSERT_EQUAL( + ParseResult::Malformed, + parseReadResponse(frame, sizeof(frame), 0x01, kReadHoldingRegisters, regs, 4, resp)); +} + +// The function-code check on the write SUCCESS path. It was covered only through the exception +// path (test_write_exception_with_a_foreign_function_is_refused), which reaches a different +// branch -- parseExceptionFrame. A plain, well-formed echo for a function we never sent would +// otherwise be accepted as confirmation of our write: on a multidrop bus, that is somebody +// else's reply being read as our setpoint landing. +static void test_a_write_reply_echoing_a_foreign_function_is_refused() { + uint8_t frame[8]; + frame[0] = 0x01; + frame[1] = kWriteMultipleRegisters; // 0x10, and we sent 0x06 + frame[2] = 0x00; + frame[3] = 0x10; + frame[4] = 0x00; + frame[5] = 0x01; + const uint16_t crc = crc16(frame, 6); + frame[6] = static_cast(crc & 0xFF); + frame[7] = static_cast((crc >> 8) & 0xFF); + + WriteResponse resp; + TEST_ASSERT_EQUAL( + ParseResult::WrongFunction, + parseWriteResponse(frame, sizeof(frame), 0x01, kWriteSingleRegister, resp)); +} + void run_modbus_rtu() { RUN_TEST(test_crc_matches_the_canonical_vector); RUN_TEST(test_crc_second_vector); @@ -310,6 +358,8 @@ void run_modbus_rtu() { RUN_TEST(test_write_single_echo_is_validated); RUN_TEST(test_write_exception_is_reported); RUN_TEST(test_write_exception_with_a_foreign_function_is_refused); + RUN_TEST(test_a_read_reply_with_an_odd_byte_count_is_malformed); + RUN_TEST(test_a_write_reply_echoing_a_foreign_function_is_refused); RUN_TEST(test_write_exception_from_another_unit_is_refused); RUN_TEST(test_write_exception_with_a_bad_crc_indicts_the_cable); } diff --git a/test/test_register_map/test_main.cpp b/test/test_register_map/test_main.cpp index c612374..4a7875a 100644 --- a/test/test_register_map/test_main.cpp +++ b/test/test_register_map/test_main.cpp @@ -488,10 +488,22 @@ static void test_a_writable_driver_flips_the_read_only_register() { map.update(*store.snapshot(), BridgeInfo{}, diag.snapshot(), g_now); TEST_ASSERT_EQUAL_UINT16(0, map.at(reg::kDriverReadOnly)); - const bool anyWriteBit = map.at(reg::kCapabilitiesWrite) || map.at(reg::kCapabilitiesWrite + 1) || - map.at(reg::kCapabilitiesWrite + 2) || - map.at(reg::kCapabilitiesWrite + 3); - TEST_ASSERT_TRUE(anyWriteBit); + // The registers must carry the driver's WRITE set, not merely something non-empty. This + // was TEST_ASSERT_TRUE(anyWriteBit) -- an OR across the four registers -- and mutation + // testing published the READ bitmap into them instead: every working driver has a + // non-empty read set, so the OR stayed true and the one test that says which channels are + // writable agreed with the wrong bitmap. A client reading it would have been told it could + // write everything the device can report. + const auto& caps = driver.capabilities(); + const uint64_t wrote = (static_cast(map.at(reg::kCapabilitiesWrite)) << 48) | + (static_cast(map.at(reg::kCapabilitiesWrite + 1)) << 32) | + (static_cast(map.at(reg::kCapabilitiesWrite + 2)) << 16) | + static_cast(map.at(reg::kCapabilitiesWrite + 3)); + TEST_ASSERT_EQUAL_UINT64(caps.write.to_ullong(), wrote); + // Without this the assertion above could not tell the two bitmaps apart, and would go + // quietly vacuous again the day the mock's sets happened to coincide. + TEST_ASSERT_TRUE_MESSAGE(caps.read.to_ullong() != caps.write.to_ullong(), + "the fixture must declare different read and write sets"); } // The agreement below is only tested AFTER a poll. Before one -- at boot, or all night on an diff --git a/test/test_rest/test_main.cpp b/test/test_rest/test_main.cpp index 14a9768..fd65a14 100644 --- a/test/test_rest/test_main.cpp +++ b/test/test_rest/test_main.cpp @@ -646,6 +646,18 @@ static void test_a_full_bus_of_summaries_still_fits() { // far over the cap a new field pushed it instead of only that it did. TEST_ASSERT_LESS_THAN_UINT32(rest::kMaxResponseBytes, json.size()); TEST_ASSERT_EQUAL_UINT32(kMaxDevices, parse(json)["devices"].size()); + + // AND THE BOUND ACTUALLY BITES. The assertion above only says this payload is small + // enough; it says nothing about whether anything would stop a larger one. Mutation + // testing deleted the `needed > maxBytes` check in json_limits::finish() -- the single + // choke point twenty-four payload builders share -- and this test stayed green. Refusing at + // one byte under the measured size is self-calibrating: it cannot go vacuous when the + // payload grows. + std::string refused = "untouched"; + TEST_ASSERT_FALSE(rest::buildStatusPayload(state, "modbus_profile-1", makeBridge(), + r.diagnostics.snapshot(), &eversolar::descriptor(), + g_now, fleet, refused, json.size() - 1)); + TEST_ASSERT_EQUAL_STRING("untouched", refused.c_str()); } // A configured device that is not polling is the failure every mistake on the settings page @@ -1765,6 +1777,16 @@ static void test_a_capture_filled_to_its_byte_ceiling_fits_in_the_response() { std::string body; TEST_ASSERT_TRUE(rest::buildCapturePayload(report, 0, body)); TEST_ASSERT_TRUE(body.size() <= rest::kMaxCaptureResponseBytes); + + // AND THE BOUND ACTUALLY BITES. The assertion above only says this payload is small + // enough; it says nothing about whether anything would stop a larger one. Mutation + // testing deleted the `needed > maxBytes` check in json_limits::finish() -- the single + // choke point twenty-four payload builders share -- and this test stayed green. Refusing at + // one byte under the measured size is self-calibrating: it cannot go vacuous when the + // payload grows. + std::string refused = "untouched"; + TEST_ASSERT_FALSE(rest::buildCapturePayload(report, 0, refused, body.size() - 1)); + TEST_ASSERT_EQUAL_STRING("untouched", refused.c_str()); } // --- restore preview ----------------------------------------------------------------------