Skip to content

test(build): restore the fw_transport suite registration dropped by #50 - #61

Merged
srpatcha merged 1 commit into
embeddedos-org:masterfrom
Harshit-Vashisth:fix/restore-fw-transport-test-registration
Aug 30, 2026
Merged

test(build): restore the fw_transport suite registration dropped by #50#61
srpatcha merged 1 commit into
embeddedos-org:masterfrom
Harshit-Vashisth:fix/restore-fw-transport-test-registration

Conversation

@Harshit-Vashisth

Copy link
Copy Markdown
Contributor

Summary

tests/unit/test_fw_transport.c has not been compiled or run since commit
39b0925 (28 Aug). It is still in the tree, and it still passes — nothing builds it.

The merge that landed test_slot_size_bounds (#50) replaced the
test_fw_transport registration block in tests/CMakeLists.txt instead of
appending a new one, in both the add_executable/add_test section and the
valgrind target list:

-# --- test_fw_transport: UART raw/YMODEM firmware transport ---
-add_executable(test_fw_transport unit/test_fw_transport.c)
-target_link_libraries(test_fw_transport PRIVATE eboot_core)
-add_test(NAME test_fw_transport COMMAND test_fw_transport)
+# --- test_slot_size_bounds: ...

Nothing reports this. The build stays green, and ctest simply runs one fewer
test than it did the day before.

The orphaned suite is the 12-case regression set added by #51 for the UART
firmware-update transports. It covers, specifically:

  • the 4-byte raw length prefix being used unchecked, so a declared length of
    0xFFFFFFFF drove a multi-gigabyte receive loop;
  • the unbounded strlen() over YMODEM block 0, which ran off the end of the
    1026-byte block buffer on the STX path (the test picks fill byte 0x21
    deliberately: its CRC over 1024 bytes is 0xE940, so neither CRC byte is NUL
    either and nothing stopped the old scan);
  • the block number being read but never validated, so a block the sender
    retransmits after a lost ACK was written to flash twice, shifting every
    following byte of the image.

Those are the fixes that have been sitting unguarded.

Type of Change

  • test — Add or fix tests
  • build — Build system change
  • docs — Documentation

Changes

  • tests/CMakeLists.txt: restore the test_fw_transport registration, and add
    it back to the valgrind target list. test_slot_size_bounds is untouched —
    both are registered now.
  • tests/unit/test_cmake_test_registration.py: new guard. It parses
    tests/CMakeLists.txt and fails if any tests/unit/test_*.c has no
    add_executable(), or if any test executable is built without a matching
    add_test(). The eBoot CI job already runs pytest tests/, so this catches
    the same class of drop on the PR that introduces it rather than weeks later.
  • CONTRIBUTING.md: the test table said "7 unit test suites" and listed 7.
    There are 17. Updated, and the registration requirement is now written down.

Testing

  • Unit tests pass
  • New tests added

test_fw_transport, 12/12 pass against current core/fw_transport_uart.c:

=== eBootloader: Firmware Transport Unit Tests ===

  test_ymodem_valid_transfer_is_written                    [OK]
  test_ymodem_duplicate_block_is_not_written_twice         [OK]
  test_ymodem_out_of_sequence_block_is_nakd                [OK]
  test_ymodem_bad_block_complement_is_nakd                 [OK]
  test_ymodem_header_without_nul_is_bounded                [OK]
  test_ymodem_stx_header_without_nul_is_bounded            [OK]
  test_ymodem_stx_duplicate_block_is_not_written_twice     [OK]
  test_ymodem_header_size_overflow_is_rejected             [OK]
  test_ymodem_first_block_must_be_zero                     [OK]
  test_raw_valid_transfer_is_written                       [OK]
  test_raw_oversized_length_is_rejected                    [OK]
  test_raw_zero_length_is_rejected                         [OK]

12/12 tests passed

The guard was broken and watched to fail (TESTING.md: "break the code and
watch the test fail"). Reverting only tests/CMakeLists.txt:

E   AssertionError: these suites exist under tests/unit/ but no add_executable()
    in tests/CMakeLists.txt builds them, so they never run: ['test_fw_transport.c']
2 failed, 1 passed

and with the registration restored, 3 passed.

Additional Notes / Limitations

CI on this PR will be red, for reasons that predate it. eboot_core does
not compile on master right now — core/recovery.c has a duplicated
slot_size declaration, core/image_verify.c's eos_crc32 definition
disagrees with its prototype in eos_image.h, and core/ed25519_verify.c has a
duplicated SHA-512 block with the verification tail missing — all unresolved
merge conflicts. #55, #57 and #58 are open against exactly that.

To run the restored suite I built test_fw_transport.c against
fw_transport_uart.c, fw_update.c, hal_core.c and the rest of its actual
dependencies with a local stub for eos_ed25519_verify (which the transport
path never calls — these images carry sig_type = 0). That stub is not part
of this branch; the diff here is three files. Once any of the build-fix PRs
lands, this suite will run under ctest normally, and I am happy to rebase.

Two further things I noticed and deliberately left alone, since fixing them here
would collide with the open build-fix PRs:

  • CMakeLists.txt lists core/boot_log.c twice in the eboot_core source list
    (lines 83 and 87). CMake de-duplicates, so it is harmless — just untidy.
  • tests/unit/test_slot_manager.c and tests/unit/test_boot_log.c do not
    compile independently of eboot_core's breakage; that is already tracked in
    the issue titled "Two test files broken independently of eboot_core's build".

…mbeddedos-org#50

tests/unit/test_fw_transport.c has not been compiled or run since commit
39b0925. The merge that added test_slot_size_bounds replaced the
test_fw_transport registration block instead of appending a new one, in
both the add_executable/add_test section and the valgrind target list.
The source file stayed in the tree, so nothing reported it as missing:
the build stayed green and ctest simply ran one fewer test.

The suite is the 12-case regression set added by embeddedos-org#51 for the UART update
transports -- the unbounded 4-byte raw length prefix, the unbounded
block-0 filename scan on the 1024-byte STX path, and the missing YMODEM
block-number validation that let a retransmitted block be written to
flash twice. Those fixes have been unguarded since.

Restore both registrations, and add
tests/unit/test_cmake_test_registration.py, which parses
tests/CMakeLists.txt and fails if any tests/unit/test_*.c has no
add_executable(), or if any test executable is built without a matching
add_test(). The eBoot CI job already runs pytest over tests/, so this
catches the same class of drop on the PR that introduces it.

CONTRIBUTING.md listed 7 unit suites; there are 17. Update the table and
document the registration requirement.

@srpatcha srpatcha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified — and this is the fourth instance of the same merge pattern

tests/unit/test_fw_transport.c is in the tree, passes, and has not been compiled since 28 Aug because #50's merge replaced its registration block instead of appending a new one. A test that silently stops running is worse than a deleted one: the file is still there, so it reads as coverage.

Confirmed on top of #58: 16 tests before this, 17 after, 0 build errors, all passing.

Worth recording that this is not an isolated slip. The same shape has now produced four separate repairs across two repos:

where what a merge did
eBoot #58 core/sha512.c and core/rollback.c never added to CMakeLists — two merged security features compiled into nothing
eBoot, this PR test_fw_transport registration replaced rather than appended
eos #82 sync.c#55 merged after #67 deleted #67's implementation but kept its call sites
eos #82 #74's tests merged without #74's parser change

Every one is a PR that was green on its own branch, squash-merged onto a base that had moved, with nothing re-verifying the result. #58's write-up names the cause correctly, and this PR is more evidence for it.

Catching the valgrind target list as well as the add_executable/add_test block is the detail that makes this a complete fix rather than a partial one.

Verification

Merged onto origin/master + #58; configure OK, 0 build errors, ctest 17/17 with test_fw_transport among them.

Not verified against master alone — master does not build; #58 repairs it. This PR also needs #58 to land first.

Blocker outside this PR

eBoot sets required_signatures: true on master and your commits are unsigned, as are every contributor's here. That makes this unmergeable regardless of review. Flagged to the maintainer as a policy decision; nothing for you to fix.

@srpatcha
srpatcha merged commit 06d3fd3 into embeddedos-org:master Aug 30, 2026
srpatcha pushed a commit to furqan72672/eBoot that referenced this pull request Aug 31, 2026
Master stopped building/testing clean again after several PRs (embeddedos-org#60,
embeddedos-org#61, embeddedos-org#64, embeddedos-org#67, embeddedos-org#69, embeddedos-org#71) landed back-to-back without an integration
build between them:

- tests/CMakeLists.txt: embeddedos-org#61 registered test_fw_transport without the
  eboot_ prefix embeddedos-org#71's namespace guard now requires, so configure
  aborted with "Test target 'test_fw_transport' is not namespaced."
  Renamed the target to eboot_test_fw_transport (add_test NAME stays
  test_fw_transport per the guard's own guidance).
- tests/CMakeLists.txt: the valgrind foreach block still referenced
  the pre-embeddedos-org#60/embeddedos-org#71 bare target names (test_bootctl, etc.) in
  $<TARGET_FILE:...>, which no longer resolve now that every target is
  eboot_-prefixed. Fixed the generator-expression reference while
  leaving the valgrind_${TEST_NAME} test labels unchanged.
- core/recovery.c: recovery_handle_write() called
  eos_recovery_write_in_range(base, slot_size, ...) with slot_size
  never declared -- embeddedos-org#69 introduced the call but the counterpart
  eos_hal_slot_size(slot) lookup (mirroring the existing
  eos_hal_slot_addr(slot) line right above it) never made it in.
- tests/unit/test_slot_manager.c: two versions of the file's fixture
  and test bodies had been spliced together by a merge (duplicate
  globals/slot_index, an unused old RUN macro next to the real TEST
  macro), and the TEST macro's simulated-flash fixture
  (sim_flash/sim_tick/sim_ops) was referenced but never defined
  anywhere in the file. Rebuilt the file as one coherent suite
  matching its own doc comment and every sibling test file's
  sim_board_ops_t convention; converted the three tests that were
  still plain functions to the same TEST() macro as the rest for
  consistency. Also gave sim_flash_erase real erase_result/
  erased_addr/erased_size bookkeeping -- the generic version copied in
  ignored those script variables entirely, which the erase test relies
  on.

Verified: cmake configure + build, 0 errors. ctest 17/17 (non-valgrind)
and 32/32 including valgrind, all passing -- including
test_recovery's real HAL-level exercise of the new slot_size bounds
check and test_slot_manager's erase-failure/erase-bookkeeping cases.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VvWBEZhDegTQMaqVtry2mM
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants