Skip to content

Fix: compile the run-drain test against the current WorkspaceManager ABI - #2463

Merged
ChaoWao merged 1 commit into
hw-native-sys:mainfrom
ChaoWao:fix/compile-the-run-drain-test-against-the-current
Sep 28, 2026
Merged

ChaoWao merged 1 commit into
hw-native-sys:mainfrom
ChaoWao:fix/compile-the-run-drain-test-against-the-current

Conversation

@ChaoWao

@ChaoWao ChaoWao commented Sep 28, 2026

Copy link
Copy Markdown
Collaborator

Summary

main does not compile the ut suite. test_run_drain_result_separation.cpp
is written against a WorkspaceManager API that no longer exists:

test_run_drain_result_separation.cpp:62: error: invalid user-defined conversion
  from '<lambda(void*, void*)>' to 'ReleaseOutcome (*)(void*, void*, int*)'
test_run_drain_result_separation.cpp:112: error: no matching function for call to
  'WorkspaceManager::configure(const uint64_t&, Backend)'
test_run_drain_result_separation.cpp:148: error: (same)

Three changes bring the fake back in line, and nothing else:

  • configure(budget, ops) → configure(ops) + set_limit(budget), keeping
    the same 1 MiB limit both cases configured before.
  • Backend::release takes (ctx, base, int *platform_rc) and returns
    ReleaseOutcome.
  • The fake's release returns Freed and leaves platform_rc untouched,
    which is what its old return 0 meant — it has no failure path.

The shape now matches the FakeBackend in test_workspace_manager.cpp, which
is the same backend already written against the current signatures.

How this reached main

Neither change is at fault on its own:

#2452 branched before #2455 landed, so its CI never saw this file, and #2455's
CI never saw the new signatures. ci.yml triggers on pull_request only —
there is no post-merge CI on main — so the collision was invisible until the
next PR branched from a main containing both. The last green ci.yml run of
any branch (refactor/dfx-tools-runtime-split, 00:37) predates both merges.

Found from #2461's red ut, and reproduced at plain upstream/main
(410502f7) to confirm it is not that PR's.

Validation

  • Full non-hardware ctest at 410502f7 + this change: 263/263 pass
    (before it, the suite does not build).
  • clang-format clean.

Test-only; no product code touched.

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: f0d6801d-e1e2-438e-90bb-83ab72effda6

📥 Commits

Reviewing files that changed from the base of the PR and between 410502f and 1f6b444.

📒 Files selected for processing (1)
  • tests/ut/cpp/common/platform/test_run_drain_result_separation.cpp

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The drain-result tests update the fake backend to use the workspace manager’s release-outcome interface. Both tests now configure the backend and memory budget separately. Existing drain-result and workspace-state assertions remain unchanged.

Changes

Drain result tests

Layer / File(s) Summary
Fake backend and test setup
tests/ut/cpp/common/platform/test_run_drain_result_separation.cpp
The fake release callback forwards the platform-result pointer, and the fake release method returns ReleaseOutcome::Freed. Both tests configure the backend before setting the memory budget.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~5 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 1f6b4

This test-only API update preserves the intended drain-result checks and presents no concrete merge-blocking risk.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description check ✅ Passed The description directly explains the test-only API updates, compilation issue, validation results, and reason for the change.
Title check ✅ Passed The title clearly and concisely identifies the main change: updating the run-drain test to compile against the current WorkspaceManager ABI.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

A rabbit checks the drain result with care,
The fake release sends its pointer there.
“Freed,” it says, as blocks depart,
The budget follows, set apart.
The workspace tests keep their state,
And nibble clover while they validate.

Comment @coderabbitai help to get the list of available commands.

`WorkspaceManager::configure` takes the backend alone and `set_limit` carries
the byte limit separately, and `Backend::release` reports a `ReleaseOutcome`
with the platform code written through an out-parameter. This test still
called the two-argument `configure` and gave `release` the old
`int (void *, void *)` shape, so `ut` did not compile at all — three errors
before any case ran.

The fake's behaviour is unchanged: it never fails a release, so it returns
`Freed` and leaves `platform_rc` untouched, and the two cases keep the same
1 MiB limit they configured before. The shape matches the fake in
test_workspace_manager.cpp, which is the same backend written against the
current signatures.

Neither side is at fault on its own. The signature change and the test landed
five minutes apart in merge order, and `ci.yml` triggers on `pull_request`
only, so no run ever saw them together until the next PR branched from a main
containing both.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@ChaoWao
ChaoWao merged commit 50fb75b into hw-native-sys:main Sep 28, 2026
15 of 17 checks passed
@ChaoWao
ChaoWao deleted the fix/compile-the-run-drain-test-against-the-current branch September 28, 2026 06:16
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.

1 participant