Skip to content

chore(ci): chat-delegate unit tests never run in CI #614

Description

@sanity

Problem

The chat-delegate crate carries roughly 39 unit tests across three modules:

  • delegates/chat-delegate/src/handlers.rs (mod tests) — ~15 tests driving the real
    ChatDelegate::process dispatch: store/get/list/delete, CAS generation conflicts,
    origin validation, handle_store_signing_key.
  • delegates/chat-delegate/src/subscription/tests.rs — 13 tests over
    handle_ensure_room_subscription, member-set comparison, derive_room_secret.
  • delegates/chat-delegate/src/versioning.rs — 11 tests over the CAS envelope
    primitives.

None of them run in CI. Makefile.toml defines test-chat-delegate (rolled into
cargo make test), but no workflow ever invokes it. Grepping .github/workflows/*.yml
for chat-delegate finds only path-trigger filters in check-cli-wasm.yml and
check-delegate-migration.yml, and neither runs cargo test
check-delegate-migration.yml only runs scripts/check-migration.sh, a WASM byte-hash
comparison.

So these tests gate merges only when a developer happens to run cargo make test
locally. river-core, river-ui, riverctl, and room-contract all have explicit
CI steps in build.yml; the delegate is the gap.

This surfaced while adding migration regression tests for #612 — the delegate side of
the migration path (the key_index behavior that makes a secret discoverable to the
ListRequest probe) has tests, but they would not have caught a regression in CI.

Suggested fix

Add a step to the build job in .github/workflows/build.yml, alongside the existing
per-crate test steps:

    - name: Test chat-delegate
      env:
        RUST_MIN_STACK: 8388608
        CARGO_TARGET_DIR: ${{ github.workspace }}/target
      run: cargo test -p chat-delegate

(The Makefile.toml task pins --target x86_64-unknown-linux-gnu with a separate
target dir because the crate's default target is wasm; whether CI needs the same
pinning should be checked when wiring it up.)

Note on what this would and would not catch

Worth knowing before assuming full coverage: on native targets DelegateCtx::default()
is a stub from freenet-stdlib where set_secret is a no-op and get_secret always
returns None (documented at handlers.rs:189-198 and subscription/tests.rs:1-30).
So these tests exercise dispatch and pure-value logic, not storage round-trips — a
"store then get returns the stored bytes" assertion is not currently expressible. The
existing tests work around this by testing pure functions over Option<&[u8]>. Wiring
them into CI is still worth doing; it just should not be read as end-to-end delegate
storage coverage. A real in-memory DelegateCtx fake would need upstream
freenet-stdlib changes.

Related: #612 (delegate migration hazard), #613 (the migration regression tests).

[AI-assisted - Claude]

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions