core+qt: public seams for the four detail:: reach-ins the ladder testkit needed - #79
Merged
Conversation
Yaraslaut
force-pushed
the
issue-55-testkit
branch
2 times, most recently
from
August 13, 2026 07:25
2090095 to
c4e48ad
Compare
…kit needed Closes #55: each of the four use cases the issue names now has a public seam instead of requiring test code to name a `detail::` type. 1. `morph::async::detail::CompletionState<T>` -> `Completion<T>::makeSettleable(execPtr)`. A new static factory returns a `{Completion<T>, Completion<T>::Promise}` pair sharing one freshly allocated state: the `Completion<T>` is exactly what `then()`/`onError()` observe, and the paired `Promise` exposes `resolve()`/`reject()` to settle it on demand -- standing in for a full `Bridge`/`IBackend` round trip in a test -- without ever naming `morph::async::detail::CompletionState<T>`. `Promise`'s constructor is private and `friend`ed only to `Completion<T>`. `resolve()`/`reject()` are no-ops on an already-settled state or a moved-from `Promise`, mirroring `Completion<T>::then()`/`onError()`'s existing null-state no-op. 2. `morph::exec::detail::StrandExecutor`/`ModelId` -> a documented public interleaving-test harness built from existing public API, no new library surface needed. `RemoteServer` funnels every task it ever dispatches -- both the top-level `handle()` post and its internal `StrandExecutor`'s per-model dispatch -- through the single `IExecutor` it was constructed with, and its wire replies already carry model identity as a plain `uint64_t` (`wire::Envelope::modelId`), never `ModelId`. Added `morph::testing::StepExecutor` to `tests/test_support.hpp` (a queue-and single-step `IExecutor`: `runOne()`/`runAll()`/`pending()`) and a pair of tests demonstrating a fully deterministic, hand-stepped interleaving harness against a real `RemoteServer` -- same-model ordering preserved, different-model work interleaved on the test's own schedule -- using only public vocabulary. Documented in docs/spec/core/executor.md next to `StrandExecutor`'s own section. 3. `morph::bridge::detail::HandlerBinding` -> already closed by #60's `Bridge::isBound()`/`whenBound()` and `BridgeHandler::isBound()`/ `whenBound()` (merged in from issue-cluster-g-async-registration, not yet on master at the time of this branch). No new code needed for this seam; verified the existing predicate/awaitable pair covers the "observe whether an async registration has completed" use case without reaching into `HandlerBinding`'s internal `currentId` field. 4. `morph::model::detail::defaultDispatcher()`/`defaultRegistry()` -> two new `QtWebSocketBackend` constructor overloads, `(serverUrl, tls, cfg)` and `(serverUrl, cfg)`, that delegate to the existing constructor with dispatcher/registry defaulted internally. `QtWebSocketBackend` never actually uses those two parameters (model construction is delegated to the server), so a caller who only wants to set `cfg` (e.g. `Config::asyncRegistrationEnabled`) no longer has to spell out `morph::model::detail::defaultDispatcher()`/`defaultRegistry()` just to reach the parameters positioned after them. Built with -DMORPH_BUILD_QT=ON; full suite green (morph_tests: 8563 assertions / 874 cases, morph_qt_tests: 447 assertions / 62 cases). Files: include/morph/core/completion.hpp, include/morph/qt/qt_websocket_backend.hpp, tests/test_support.hpp, tests/test_completion_promise.cpp, tests/test_remote_step_interleaving.cpp, tests/qt/test_qt_websocket.cpp, tests/CMakeLists.txt, docs/spec/core/completion.md, docs/spec/core/executor.md, docs/spec/core/backend.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- tests/CMakeLists.txt: register test_bridge_pending_calls.cpp, which was added in c76b849 but never wired into the morph_tests target and so never compiled or ran (a real cpp-review Blocker: a test's absence from the build silently drops the coverage it claims to give). - include/morph/qt/qt_websocket_backend.hpp: fix registerModelAsync's doc comment for the never-connected-then-destroyed case. It claimed a queued registration is dropped on destruction "without invoking either callback", but ~QtWebSocketBackend calls cancelPending(), which does invoke onError for every queued entry (by design, per its own comment) so the caller is never left waiting forever. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ehavior, not history Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tead of hanging An unbounded 'while (runOne())' loop has no way to distinguish a legitimately-draining queue from a task that keeps re-posting more work to itself -- a strand bug, or a harness misuse, would spin runAll() forever with no assertion failure and no compile-time signal, only a hung test process indistinguishable from a CI timeout. runAll() now takes a maxSteps bound (generous default: 10,000) and throws std::runtime_error if it's reached, naming runOne() as the way to step through and find the runaway task. Adds a regression test with a small bound proving a self-re-posting task throws immediately rather than hanging. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Yaraslaut
force-pushed
the
issue-55-testkit
branch
from
August 13, 2026 07:57
c4e48ad to
2e1b142
Compare
…verload The other new constructor overload, (serverUrl, cfg), already had a dedicated test; this three-argument one -- letting a caller pass a tls configuration without naming the dispatcher/registry pair -- had none. Every existing call site in this file uses either the full four-argument constructor with explicit defaultDispatcher()/defaultRegistry(), or the bare (url) shorthand, or the (url, cfg) overload -- never (url, tls, cfg) directly. Mirrors the existing Config-only overload test: connects, registers a handler asynchronously, executes an action, confirms the round trip works -- proving the overload resolves and delegates correctly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Yaraslaut
marked this pull request as ready for review
August 13, 2026 08:03
…y, not notifyCount notifyCount is the wrong signal: OrderModel::onBackendChanged() increments it BEFORE draining the queue, so polling on notifyCount reaching 1 only proves the call started, not that the drain finished. This raced the drain itself and reproduced on CI as both a plain assertion failure (queue.drain().empty() intermittently still false) and, once, a genuine UBSan misaligned-member-call report -- calling execute() concurrently with a model instance mid-teardown during the same race window. InMemoryOfflineQueue::drain() is a thread-safe, non-destructive snapshot read (per its own doc comment), so polling it directly is safe and becomes empty at the exact moment every item is drained and marked done -- the actual signal these tests need, with no race window. Verified 20x back-to-back locally with no failures. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ion to avoid an ODR collision Root cause of the real CI failure: this file (added by this PR) declared its own file-scope OrderModel/OrderAction with external linkage -- identical simple names to test_conflict_resolution.cpp's own, entirely unrelated, pre-existing OrderModel (which has onBackendChanged(), notifyCount, offline-queue draining, none of which this file's stub type has). Two external-linkage types with the same name and different definitions is a One-Definition-Rule violation the linker does not diagnose; which definition ends up linked into which translation unit is compiler/link-order dependent. Confirmed via targeted tracing: in the affected builds, LocalBackend's _changeAware set was empty after registering a model built from test_conflict_resolution.cpp's own modelFactory -- i.e. the linker had resolved BackendChangedNotifiable<OrderModel> using this file's bare stub definition instead of the real one, so notifyBackendChanged() never posted to onBackendChanged() at all and the offline queue never drained. Reproduced deterministically (100% of runs, not flaky) on Linux/clang-ubsan and on real CI across nearly every Linux job; verified absent on 10/10 runs after this rename, both under WSL/clang-ubsan and matching CI's own per-test-case invocation pattern. Renamed to StepILOrderModel/StepILOrderAction (matching this file's own StepIL_* wire-typeId convention already in use), eliminating the collision without touching test_conflict_resolution.cpp at all. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #55.
Four use cases previously required reaching into morph
detail::namespaces; each now has a public seam:Completion<T>promise —Completion<T>::makeSettleable(IExecutor*)returns aCompletion<T>/Promisepair a test can resolve on demand, without a fullBridge/IBackendround trip.morph::async::detail::CompletionState<T>no longer needs to appear in caller code.StrandExecutor/ModelIdinterleaving harness — addedmorph::testing::StepExecutor(queue-and-single-stepIExecutor) demonstrating deterministic hand-stepped interleaving against a realRemoteServer, using zerodetail::symbols.HandlerBinding::registered()/isBound()— already satisfied by No "registration settled" seam: dispatch issued right after connect fails "handler not bound" #60 (in the async-registration PR, core+qt: complete the async model-registration lifecycle (queueing, settled seam, callId hardening, Sharing-aware dispatch, async promote, connection scope) #71):Bridge/BridgeHandler::isBound()/whenBound(). Not duplicated here.QtWebSocketBackend::Configconstructor overload — two new delegating overloads that defaultdispatcher/registryinternally, so a caller who only wantsConfig::asyncRegistrationEnableddoesn't have to spell outmorph::model::detail::defaultDispatcher()/defaultRegistry().This branch merges
issue-cluster-b-completion-bridge(#70, forCompletion<T>'s post-#59 composing shape) andissue-cluster-g-async-registration(#71, for the post-#60BridgeHandlershape) into itself, since seams #1 and #3 needed to build against their latest shape rather than stalemaster. Merge #70 and #71 first, then rebase this branch (or re-target it) to drop the merge commits before landing, to avoid duplicate history in the final diff. Opening as a draft for that reason.Review
Ran
contour-workflows:cpp-reviewagainst the branch diff (real skill invocation). Findings applied in the follow-upreview:commit:tests/test_bridge_pending_calls.cpp(inherited from the core: Completion<T> composes onError/then handlers; add Bridge::pendingCalls() #70 merge) was never wired intotests/CMakeLists.txt— fixed here too since it was present on this branch.registerModelAsyncdoc/code contradiction (inherited from the core+qt: complete the async model-registration lifecycle (queueing, settled seam, callId hardening, Sharing-aware dispatch, async promote, connection scope) #71 merge) — fixed here too.Test plan
morph_tests.exe: 8563 assertions / 874 cases, all passing.morph_qt_tests.exe: 447 assertions / 62 cases, all passing.🤖 Generated with Claude Code