Cull the comments in the firmware update tests #57
Loading…
Reference in a new issue
No description provided.
Delete branch "chore/cull-test-comments"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Drops 282 of the comment lines this branch added to
test/andtools/vector_cases.h, down to 38. Most restated the test's own name or the rationale already in the spec, the ticket or the pull request; what is left is a bare spec pointer per file, a note on each unobvious fixture, and the curation reasons on the shared vector cases.Two tests carried their meaning in a comment rather than their name, so they were renamed instead:
test_the_inputs_that_fill_a_block_exactlyis nowtest_an_input_one_short_of_a_block_and_one_that_fills_it, andFakeTransport::perReadis nowimageBytesPerRead.No behaviour change: 412 cases before and after, both node environments and
host_simbuild, andgen_vectorsreproducesvectors/frames.jsonandtest/test_vectors/expected_bytes.hbyte for byte.Two shortened comments became wrong or lost the half that was load-bearing, and one pre-existing error got carried through a rewrite. Rest is line-level.
Also: the rewritten one-liners run 86-118 characters, where nothing on
mainexceeds 82..clang-formathasColumnLimit: 160so it will not catch this. Rewrap at ~80 — affectstest_sha256.cpp:1,test_firmware_manifest.cpp:33,test_firmware_updater.cpp:19,test_mcp2515_tx.cpp:26,test_update_failed_state.cpp:223,test_wifi_manager.cpp:129,263,713,vector_cases.h:89,278,553,565.The three redzone notes (
parseExactly,feed,offer) pin the same trap but no longer read the same: two say "so the sanitiser's redzone follows that length", the manifest one adds "rather than a literal's NUL". Make them identical.@ -491,3 +489,2 @@// The spec names the CRC32 variant, and a receiver that picks a different one// rejects every transfer. This is the check string the standard is quoted with.// "123456789" and 0xCBF43926 are the check values the standard is quoted with.This now restates the two literals directly below it, and
test_crc32_matches_the_standardalready says the rest. The only unrecoverable bit was which CRC-32 variant the spec names — either say that, or delete the comment.@ -20,3 +18,2 @@// The digest is the one the image below really hashes to; test_sha256 pins that// separately, so a mismatch here is the updater's doing.// The digest is the real hash of the image below, so a mismatch here is the updater's doing.The cut half was the useful half. This digest literal has to stay equal to the one in test_sha256's
test_the_image_the_updater_tests_serve, and nothing now links the two files. As written the comment asserts something a reader cannot check. Restore the pointer:// test_sha256 pins this digest against the same image.@ -30,3 +25,2 @@// The image the fake server serves: every byte its own offset, so a sink that// drops or reorders a chunk shows up.// Every byte its own offset, so a dropped or reordered chunk shows up.First clause restates the one-line body (
offset & 0xFF). Keep the why only:// So a dropped or reordered chunk shows up.@ -1,6 +1,4 @@// The digest an image is verified against, so the published NIST vectors are// what pin it: a hash that is wrong in the same way on both sides of a// comparison still matches itself. Spec section 11.// The expected digests are the published NIST vectors, not this implementation's own output. Spec section 11.This is now a false absolute.
test_the_image_the_updater_tests_serve(line 122) pinsc8f5d034…, which is the digest of the 4096-byte pattern — this implementation's own output, not a published vector. The old wording was vague enough to survive that; this one is not. Something like "Expected digests are the published NIST vectors, never regenerated from this implementation" keeps the trap without the overclaim.@ -66,3 +59,1 @@// 63 bytes leaves room for the 0x80 and nothing else, and 64 fills the block// exactly, so both push the whole length field into a block of its own.void test_the_inputs_that_fill_a_block_exactly() {void test_an_input_one_short_of_a_block_and_one_that_fills_it() {Rename is accurate (63 and 64 against a 64-byte block) and beats the old name, but it is named for the input while its sibling at line 50 is named for the mechanism.
test_the_inputs_that_push_the_length_field_into_a_block_of_its_ownwould match the file and carry the why the deleted comment held.@ -268,3 +262,2 @@// Installed, with the manager not yet round to seeing it: the window an inbound// frame lands in, since Node::loop drains frames before WifiManager::loop.// Installed but not yet seen by the manager: Node::loop drains frames before WifiManager::loop.The colon no longer connects. The cut clause ("the window an inbound frame lands in") was what tied the install state to the drain order; without it the second half is a non-sequitur and the helper's reason for existing is gone.
@ -568,3 +564,2 @@{// The short frame that ends a transfer: a decoder reading six bytes// whatever the DLC says takes two bytes of nothing into the key.// The short final frame: a decoder ignoring the DLC takes two bytes of nothing into the key."two bytes of nothing" is wrong — should be four.
sessionContentis 14 bytes atbulkChunkLength6, so the final frame carries 2 payload bytes at dlc 4; a decoder reading six regardless takes four bytes past the dlc. The error predates this branch, but the line was rewritten here, so fix it or drop the count.Round two. One thing still wrong, one trivial.
test/test_boot_commit/test_boot_commit.cpp:1is 84 characters, the only line still over the 82 the rest now sits at.Round-one points 2, 3, 5, 6, 7 and 8 are all fixed, and the CRC32 wording quotes section 8.9 correctly — the spec names IEEE 802.3 with polynomial
0xEDB88320at that section.@ -1,6 +1,5 @@// The digest an image is verified against, so the published NIST vectors are// what pin it: a hash that is wrong in the same way on both sides of a// comparison still matches itself. Spec section 11.// The expected digests are the published NIST vectors, bar the updater image atStill not true — the exception list is one short.
test_the_inputs_that_push_the_length_field_into_a_block_of_its_own(line 59) feedspatternOf, bytes 0x00-0x3F, sowant63andwant64are digests of that pattern and not published vectors either. Checked independently: sha256 of bytes 0..62 is29af2686…and of 0..63 isfdeab9ac…, matching the literals.The inventory form is the problem — it also goes stale the next time a test is added, and "at the foot of the file" breaks as soon as something is appended. State the rule instead: something like "Every expected digest is computed independently of this implementation, never regenerated from it. Spec section 11." That covers the NIST vectors and the two pattern digests without naming either.
One line went backwards in the rewrap; everything else is clean. The sha256 header now says the right thing and covers every case in the file.
@ -7,2 +1,2 @@// completed. Writing transmitCompleted_ from here is the same latch the MCP2515// and TWAI backends set.// Spec section 11. CanBus is defined here rather than linked, as in// test_node_boot; writing transmitCompleted_ is the latch the real backends set."the latch the real backends set" is not true of all of them —
can_bus_socketcan.cpp:145leavestransmitCompleted_deliberately alone, because a write to vcan succeeds with nobody listening. Naming MCP2515 and TWAI was accurate and the precision was lost for two columns. Rewrap instead:// Spec section 11. CanBus is defined here rather than linked, as in // test_node_boot; writing transmitCompleted_ is the same latch the MCP2515 // and TWAI backends set.Nothing outstanding.