Skip to content

[AIT-1131] fix(uts): align objects unit tests with reconciled specs and close audit gaps#2257

Merged
sacOO7 merged 1 commit into
uts-liveobjectsfrom
fix/uts-liveobjects-unit-tests
Jul 10, 2026
Merged

[AIT-1131] fix(uts): align objects unit tests with reconciled specs and close audit gaps#2257
sacOO7 merged 1 commit into
uts-liveobjectsfrom
fix/uts-liveobjects-unit-tests

Conversation

@sacOO7

@sacOO7 sacOO7 commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Intent

The objects unit-test specs were reconciled with the canonical LiveObjects spec in ably/specification#499, and an exhaustive spec↔test audit of the derived ably-js suite surfaced a handful of verification gaps (dropped assertions, one vacuous assertion, tests exercising the wrong scenario). This PR brings the derived unit tests in line with the reconciled specs and closes every audit finding, so the suite verifies exactly what the specs prescribe.

Two structurally new tests are added; everything else is assertion-level convergence. Suite: 309 passing, 0 failing, 4 pending (all four documented in deviations.md).

Changes

1. Spec reconciliation (ably/specification#499 logic changes)

  • RTO23e (ensure-active-channel) — new test RTO23e/get-reattaches-detached-0: get() on a DETACHED channel re-attaches per RTL33b and resolves with the root PathObject. The existing get()-on-FAILED test is relabeled to RTO23e/get-rejects-failed-0 (rejects 90001 per RTL33c) and the now-duplicate old RTO23b body is removed.
  • RTO25b — access preconditions are exercised through a real access method, [...root.keys()] (the precondition throws on first iteration of the generator), on both DETACHED and FAILED — the FAILED variant is the second new test.
  • RTO20e/RTO20e1 — the sync-wait test now asserts mid-sync state (operation not settled, value unchanged while SYNCING) before completing the sync; the RTO20e1 Test ID is corrected to fails-on-channel-detached-0 (the ERROR→FAILED adaptation remains, documented).
  • RTO24c1 / RTO24b2a1 (depth semantics) — the spec adopted ably-js's self-counts-as-depth-1 model, so the RTO24c1 test is rewritten to depth: 1 with the spec's new stimuli, and the two path_object_subscribe depth tests go spec-verbatim (single-candidate COUNTER_INC grandchild stimuli, expected counts drop 3→2). Three inline deviation adaptations retire as a result.
  • Quiescence controls — negative "listener did not fire" assertions now await a positive control delivery through the same dispatch before asserting silence (RTINS16f, both depth tests, the RUN_DEVIATIONS-gated RTO24b2b); RTINS16g asserts identity on the delivered event's object rather than the pre-existing handle (the exact vacuous form the spec fixed).
  • Canonical harness constantsstandard_test_pool.ts exports SITE_CODE and ackSerial(msgSerial, i); the echo-dedup, ack-after-echo and RTO5c9 replay tests reference them instead of hardcoded literals.

2. Audit fixes (verification gaps found by the spec↔test audit)

  • RTO3 now tests what the spec asks: a fresh, unsynced pool initializes with a zero-value root LiveMap (previously ran against a synced pool).
  • RTO4b captures the root LiveMap's internal notifyUpdated so the spec's update-diff (name: 'removed') and objectMessage-unpopulated assertions are genuinely verified — the previous updates[0].objectMessage assertion was vacuous (events expose .message, so it passed unconditionally).
  • RTO5c9 regains the spec's serial-replay steps, proving appliedOnAckSerials is actually cleared on re-sync rather than dedup-rejected.
  • update.noop == true assertions restored at all 7 rejection sites (6 in live_map, 1 in live_counter) via the existing captureNotifyUpdated helper; statusCode == 400 restored at 3 sites; object_id and RTPO13c5 UTS tags aligned with the spec's Test IDs (traceability).
  • RTLO4b4c1 now exercises the genuine noop suppression path (already-merged COUNTER_CREATE) with the spec's follow-up control increment and length == 2 assertion.

3. deviations.md

Retired the entries superseded by the reconciled specs (get()-re-attaches-on-DETACHED, RTO24c1 depth semantics, RTLM24 strict-> comparison — all now spec-mandated behavior); reframed RTLM9b to the throw-instead-of-false family; added the new RTLC9h entry (ably-js applies missing-number COUNTER_INC as NaN instead of nooping) and registry entries for the four skipped tests, plus a null/undefined language-mapping umbrella entry.

Spec defect found during implementation

Implementing the spec's canonical ack_serial = "ack-{m}:{i}" verbatim broke 7 tests: the value sorts before the standard pool's "t:0" entry timeserials under the string LWW comparison (RTLM9), so locally applied MAP_SETs on pool entries are rejected as stale (the new RTO24c1 serials "s:1"/"s:2" have the same problem). Any strictly-deriving SDK will hit this. This PR uses the sort-safe t:{msgSerial+1}:{i} form inside the spec-named ackSerial() helper; the corresponding spec-side fix is prepared on the spec branch to accompany ably/specification#499.

Validation

  • npm run test:uts:unit scope: 309 passing, 0 failing, 4 pending — the pending four are deliberate, documented skips.
  • Mechanical // UTS: tag ↔ spec Test ID set-diff against the reconciled specs is clean: every spec Test ID has a derived test and every tag resolves, with only the documented exceptions (RTLMV4b, untranslatable to JS).
  • Prettier clean.

Related

🤖 Generated with Claude Code

Summary by CodeRabbit

Summary by CodeRabbit

  • Documentation

    • Expanded UTS deviation notes to clarify coverage gaps, refined subscription argument order, and updated channel clear()/lifecycle edge-case expectations.
  • Bug Fixes

    • Prevented cleanup routines from deleting the root object when it isn’t present in synced object IDs.
  • Tests

    • Updated object/map/path/channel tests with stronger, observable assertions (noop suppression, dispatch counts, detach/failure scenarios, depth filtering with quiescence waits).
    • Introduced shared test constants/helpers to standardize serial and site code usage.
    • Adjusted timeouts and error assertions for more reliable validation.

@coderabbitai

coderabbitai Bot commented Jul 7, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

This PR updates LiveObjects UTS tests, shared serial helpers, synchronization and subscription assertions, channel lifecycle scenarios, root-object preservation, timeout handling, and deviation documentation.

Changes

UTS test and deviation documentation updates

Layer / File(s) Summary
Shared serial and site-code helpers
test/uts/objects/helpers/standard_test_pool.ts
Adds canonical serial/site-code helpers for synced-channel handshakes, ACKs, and object-operation fixtures.
Object state and noop assertions
test/uts/objects/unit/live_counter.test.ts, test/uts/objects/unit/live_map.test.ts, test/uts/objects/unit/objects_pool.test.ts, test/uts/objects/unit/parent_references.test.ts
Captures noop payloads, adjusts clear semantics, strengthens reattach assertions, and updates sync fixtures.
Instance and LiveObject subscriptions
test/uts/objects/unit/instance.test.ts, test/uts/objects/unit/live_object_subscribe.test.ts
Validates unsubscribe isolation, noop suppression, delivered object identity, and standardized operation serials.
PathObject validation and subscriptions
test/uts/objects/unit/path_object*.test.ts
Adds status-code assertions and revises depth-filtered, path-following, and dispatch-count coverage.
RealtimeObject lifecycle and synchronization
test/uts/objects/unit/realtime_object.test.ts
Adds DETACHED reattachment coverage, strengthens sync and failure paths, verifies ACK behavior after resync, and updates subscription scenarios.
Deviation and API test documentation
test/uts/deviations.md, test/uts/objects/unit/live_map_api.test.ts
Documents skipped and adapted cases for LiveMap clear, JavaScript value mapping, channel failures, serial behavior, and subscription arguments.
Runtime support adjustments
src/plugins/liveobjects/objectspool.ts, test/uts/objects/unit/object_id.test.ts, test/uts/objects/unit/value_types.test.ts
Preserves the root object during extra-object deletion, updates UTS markers, and extends a table-driven test timeout.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

  • ably/ably-js#2256: Updates the related RTO20e/RTO20e1 synchronization failure-path tests.

Suggested reviewers: paddybyers, ttypic

Poem

A rabbit checked each serial trail,
And watched the noop updates sail.
DETACHED hopped back in place,
Depth filters kept their space,
While root stayed safe inside the warren. 🐇

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: aligning objects unit tests with reconciled specs and closing audit gaps.
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.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/uts-liveobjects-unit-tests

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

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

@github-actions github-actions Bot temporarily deployed to staging/pull/2257/features July 7, 2026 12:04 Inactive
@github-actions github-actions Bot temporarily deployed to staging/pull/2257/bundle-report July 7, 2026 12:05 Inactive
@github-actions github-actions Bot temporarily deployed to staging/pull/2257/typedoc July 7, 2026 12:05 Inactive
@sacOO7 sacOO7 force-pushed the fix/uts-liveobjects-unit-tests branch from 7bd5ccb to 72104a0 Compare July 7, 2026 12:06
@github-actions github-actions Bot temporarily deployed to staging/pull/2257/features July 7, 2026 12:07 Inactive
@github-actions github-actions Bot temporarily deployed to staging/pull/2257/bundle-report July 7, 2026 12:07 Inactive
@github-actions github-actions Bot temporarily deployed to staging/pull/2257/typedoc July 7, 2026 12:07 Inactive
@sacOO7 sacOO7 force-pushed the fix/uts-liveobjects-unit-tests branch from 72104a0 to b3551af Compare July 7, 2026 12:13
@github-actions github-actions Bot temporarily deployed to staging/pull/2257/features July 7, 2026 12:14 Inactive
@github-actions github-actions Bot temporarily deployed to staging/pull/2257/bundle-report July 7, 2026 12:14 Inactive
@github-actions github-actions Bot temporarily deployed to staging/pull/2257/typedoc July 7, 2026 12:14 Inactive

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
test/uts/objects/unit/path_object_subscribe.test.ts (1)

189-228: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Raise the test timeout for this quiescence loop
Set this.timeout(5000) (or higher) on this test block; Mocha’s default 2s timeout can cut the test off before the loop’s own deadline, turning a slow dispatch into a generic timeout failure. This also applies to the matching quiescence loop below.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/uts/objects/unit/path_object_subscribe.test.ts` around lines 189 - 228,
The quiescence loop in the RTPO19c1 depth-2 subscribe test can exceed Mocha’s
default timeout, causing a generic failure before the assertion runs. Update the
test block for `subscribe()`/`flushAsync()` in `path_object_subscribe.test.ts`
to set a higher timeout via the surrounding `it(...)` callback context, and
apply the same timeout increase to the matching quiescence test below so both
`controlEvents` polling loops can complete reliably.
🧹 Nitpick comments (3)
test/uts/objects/unit/objects_pool.test.ts (1)

218-228: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Duplicated notifyUpdated-capture monkey-patch across test files.

The same capture pattern already exists as captureNotifyUpdated in live_map.test.ts. Consider extracting a shared helper (e.g. into standard_test_pool.ts) to avoid drift between the two implementations.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/uts/objects/unit/objects_pool.test.ts` around lines 218 - 228, The
`notifyUpdated` capture logic is duplicated here and in `captureNotifyUpdated`
from `live_map.test.ts`, so the test setup can drift. Extract the monkey-patch
into a shared helper in `standard_test_pool.ts` (or an equivalent common test
utility) and update `objects_pool.test.ts` to use that helper instead of
reimplementing the `rootMap.notifyUpdated` wrapper. Keep the helper centered
around the existing `getRealtimeObject`/`getPool().get('root')` flow so both
tests share the same behavior.
test/uts/objects/unit/realtime_object.test.ts (1)

1473-1504: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider extracting the deadline-based quiescence poll into a shared helper.

The while (cond && Date.now() < deadline) { await flushAsync(); } pattern is duplicated here and recurs across other files in this PR stack (e.g., path_object_subscribe.test.ts). Extracting a small waitUntil(conditionFn, timeoutMs) helper into standard_test_pool.ts (or a shared test-utils module) would reduce duplication and centralize the timeout value.

♻️ Example helper
export async function waitUntil(condition: () => boolean, timeoutMs = 5000): Promise<void> {
  const deadline = Date.now() + timeoutMs;
  while (!condition() && Date.now() < deadline) {
    await flushAsync();
  }
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/uts/objects/unit/realtime_object.test.ts` around lines 1473 - 1504, The
deadline-based quiescence polling is duplicated in this test and similar ones,
so extract it into a साझा helper such as waitUntil in standard_test_pool.ts or a
shared test-utils module. Replace the inline while loop in
realtime_object.test.ts with the helper, keeping the existing timeout behavior
and using the helper consistently from other tests like
path_object_subscribe.test.ts.
test/uts/objects/unit/instance.test.ts (1)

239-252: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Raise the timeout for this test. test:uts runs with --no-config, so the 10s timeout in test/support/root_hooks.js does not apply here. The 5s quiescence loop can still be cut off by Mocha’s default 2s limit, leaving a generic timeout instead of this assertion. Add this.timeout(6000); to this it().

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/uts/objects/unit/instance.test.ts` around lines 239 - 252, The
quiescence check in the instance test can still hit Mocha’s default timeout
because the shared test timeout setup does not apply under test:uts. Update the
relevant it() in instance.test.ts to explicitly raise the timeout (for example
by calling this.timeout(6000) at the start of the test) so the controlEvents
wait loop can complete without failing generically.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/uts/objects/unit/realtime_object.test.ts`:
- Around line 1141-1148: The `keys()` precondition tests need to actually
iterate the generator so the RTO25b check runs, rather than only constructing
the generator object. Update the relevant assertions in
`realtime_object.test.ts` to force consumption with the generator-spread pattern
used around `root.keys()`, and apply the same fix to the other `keys()` test
block referenced by this review so both the success/FAILED-channel cases
exercise the precondition path.
- Line 436: The RTO20e1 test case is still exercising a FAILED-channel flow
instead of the renamed DETACHED scenario. Update the relevant test in
realtime_object.test.ts so the setup actually transitions the channel to
DETACHED rather than sending PM_ACTION.ERROR and asserting 92008, or rename the
test back if the intended behavior is still FAILED. Use the existing RTO20e1
test block and its channel state assertions as the place to make the scenario
match the new detached-channel intent.

---

Outside diff comments:
In `@test/uts/objects/unit/path_object_subscribe.test.ts`:
- Around line 189-228: The quiescence loop in the RTPO19c1 depth-2 subscribe
test can exceed Mocha’s default timeout, causing a generic failure before the
assertion runs. Update the test block for `subscribe()`/`flushAsync()` in
`path_object_subscribe.test.ts` to set a higher timeout via the surrounding
`it(...)` callback context, and apply the same timeout increase to the matching
quiescence test below so both `controlEvents` polling loops can complete
reliably.

---

Nitpick comments:
In `@test/uts/objects/unit/instance.test.ts`:
- Around line 239-252: The quiescence check in the instance test can still hit
Mocha’s default timeout because the shared test timeout setup does not apply
under test:uts. Update the relevant it() in instance.test.ts to explicitly raise
the timeout (for example by calling this.timeout(6000) at the start of the test)
so the controlEvents wait loop can complete without failing generically.

In `@test/uts/objects/unit/objects_pool.test.ts`:
- Around line 218-228: The `notifyUpdated` capture logic is duplicated here and
in `captureNotifyUpdated` from `live_map.test.ts`, so the test setup can drift.
Extract the monkey-patch into a shared helper in `standard_test_pool.ts` (or an
equivalent common test utility) and update `objects_pool.test.ts` to use that
helper instead of reimplementing the `rootMap.notifyUpdated` wrapper. Keep the
helper centered around the existing `getRealtimeObject`/`getPool().get('root')`
flow so both tests share the same behavior.

In `@test/uts/objects/unit/realtime_object.test.ts`:
- Around line 1473-1504: The deadline-based quiescence polling is duplicated in
this test and similar ones, so extract it into a साझा helper such as waitUntil
in standard_test_pool.ts or a shared test-utils module. Replace the inline while
loop in realtime_object.test.ts with the helper, keeping the existing timeout
behavior and using the helper consistently from other tests like
path_object_subscribe.test.ts.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ec617a62-a722-4661-a170-e7243f47b3ff

📥 Commits

Reviewing files that changed from the base of the PR and between 4d0fa70 and 7bd5ccb.

📒 Files selected for processing (14)
  • test/uts/deviations.md
  • test/uts/objects/helpers/standard_test_pool.ts
  • test/uts/objects/unit/instance.test.ts
  • test/uts/objects/unit/live_counter.test.ts
  • test/uts/objects/unit/live_map.test.ts
  • test/uts/objects/unit/live_map_api.test.ts
  • test/uts/objects/unit/live_object_subscribe.test.ts
  • test/uts/objects/unit/object_id.test.ts
  • test/uts/objects/unit/objects_pool.test.ts
  • test/uts/objects/unit/path_object.test.ts
  • test/uts/objects/unit/path_object_mutations.test.ts
  • test/uts/objects/unit/path_object_subscribe.test.ts
  • test/uts/objects/unit/realtime_object.test.ts
  • test/uts/objects/unit/value_types.test.ts

Comment thread test/uts/objects/unit/realtime_object.test.ts
Comment thread test/uts/objects/unit/realtime_object.test.ts
sacOO7 added a commit to ably/specification that referenced this pull request Jul 7, 2026
…specs

Implementing the reconciled objects specs against ably-js surfaced a
serial-ordering defect: the canonical ack_serial value "ack-{msgSerial}:{i}"
and the "s:1"/"s:2" serials in the RTO24a/RTO24c1 tests sort BEFORE the
standard pool's "t:0" entry timeserials under the string LWW comparison
(RTLM9), so a strictly-derived SDK rejects those operations as stale -
every local apply-on-ACK MAP_SET on a pool entry fails, and the RTO24c1
root MAP_SET never fires its subscription.

- ack_serial redefined to the sort-safe "t:" + (msgSerial + 1) + ":" + i
  form, with a NOTE documenting the ordering constraint.
- All 8 inline auto-ACK harness copies now call ack_serial(msg.msgSerial, i)
  instead of duplicating the literal, per the helper's own "never hardcode"
  convention; the 3 literal "ack-0:0" uses in the echo-dedup and
  ack-after-echo tests reference ack_serial(0, 0).
- RTO24a/RTO24c1 serials "s:1"/"s:2" changed to "t:1"/"t:2"; the adjacent
  rationale comments rewritten to state both the site-level newness and
  entry-level LWW ordering facts.
- Removed the stale note-only Test IDs RTLC12b/increment-requires-publish-0
  and RTLM20d/echo-messages-false-0 (their sections carry no steps or
  assertions - the preconditions live in realtime_object.md RTO26) so tag
  sweeps stop reporting them as permanently missing tests.
- Added the missing RTLCV3c/no-validation-at-create-0 section to
  value_types.md (no validation at blueprint creation; rejection happens
  at evaluation per RTLCV4a).

Validated by the derived ably-js suite (ably/ably-js#2257): 309 passing,
0 failing with these values; the original values reproduce 7 failures.
@sacOO7 sacOO7 requested a review from ttypic July 7, 2026 12:53

@ttypic ttypic left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@sacOO7 sacOO7 changed the title fix(uts): align objects unit tests with reconciled specs and close audit gaps [AIT-1131] fix(uts): align objects unit tests with reconciled specs and close audit gaps Jul 8, 2026
@github-actions github-actions Bot temporarily deployed to staging/pull/2257/features July 9, 2026 17:19 Inactive
@github-actions github-actions Bot temporarily deployed to staging/pull/2257/bundle-report July 9, 2026 17:19 Inactive
@github-actions github-actions Bot temporarily deployed to staging/pull/2257/typedoc July 9, 2026 17:19 Inactive

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/uts/objects/unit/live_object_subscribe.test.ts`:
- Line 115: The Prettier failure is caused by an overly long `buildMapSet` call
inside `buildObjectMessage` in `live_object_subscribe.test.ts`; wrap that call
across multiple lines to satisfy the line-width limit while keeping the same
arguments and test behavior. Use the `buildMapSet` and `buildObjectMessage` call
sites to locate the change and apply standard Prettier multiline formatting.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e4005f35-835b-4ce7-979e-955ffc747a4a

📥 Commits

Reviewing files that changed from the base of the PR and between b3551af and b6aeca4.

📒 Files selected for processing (7)
  • test/uts/objects/helpers/standard_test_pool.ts
  • test/uts/objects/unit/instance.test.ts
  • test/uts/objects/unit/live_object_subscribe.test.ts
  • test/uts/objects/unit/objects_pool.test.ts
  • test/uts/objects/unit/parent_references.test.ts
  • test/uts/objects/unit/path_object_subscribe.test.ts
  • test/uts/objects/unit/realtime_object.test.ts
✅ Files skipped from review due to trivial changes (1)
  • test/uts/objects/unit/parent_references.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • test/uts/objects/unit/instance.test.ts

Comment thread test/uts/objects/unit/live_object_subscribe.test.ts Outdated
@github-actions github-actions Bot temporarily deployed to staging/pull/2257/features July 10, 2026 08:17 Inactive
@github-actions github-actions Bot temporarily deployed to staging/pull/2257/bundle-report July 10, 2026 08:17 Inactive
@github-actions github-actions Bot temporarily deployed to staging/pull/2257/typedoc July 10, 2026 08:17 Inactive

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
src/plugins/liveobjects/objectspool.ts (1)

51-56: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Root-preservation logic is correct; consider a Set for O(n) lookup.

The && x !== ROOT_OBJECT_ID guard correctly prevents the root object from being removed during sync, aligning with RTO5c2a. The spec comments are clear.

One performance note: objectIds.includes(x) inside filter makes this O(pool_size × objectIds_length). If sync sequences can be large, converting objectIds to a Set upfront reduces this to O(pool_size).

♻️ Proposed refactor: use Set for O(1) lookup
 deleteExtraObjectIds(objectIds: string[]): void {
   const poolObjectIds = [...this._pool.keys()];
-  const extraObjectIds = poolObjectIds.filter((x) => !objectIds.includes(x) && x !== ROOT_OBJECT_ID);
+  const objectIdSet = new Set(objectIds);
+  const extraObjectIds = poolObjectIds.filter((x) => !objectIdSet.has(x) && x !== ROOT_OBJECT_ID);

   extraObjectIds.forEach((x) => this._pool.delete(x));
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/plugins/liveobjects/objectspool.ts` around lines 51 - 56, Optimize
deleteExtraObjectIds in the object pool by constructing a Set from objectIds
before filtering, then use Set.has(x) instead of objectIds.includes(x) while
preserving the ROOT_OBJECT_ID exclusion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/plugins/liveobjects/objectspool.ts`:
- Around line 51-56: Optimize deleteExtraObjectIds in the object pool by
constructing a Set from objectIds before filtering, then use Set.has(x) instead
of objectIds.includes(x) while preserving the ROOT_OBJECT_ID exclusion.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3f18ed57-7d2a-4e65-adde-4402cdf90e75

📥 Commits

Reviewing files that changed from the base of the PR and between b6aeca4 and 6f432bf.

📒 Files selected for processing (3)
  • src/plugins/liveobjects/objectspool.ts
  • test/uts/deviations.md
  • test/uts/objects/unit/path_object_subscribe.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • test/uts/deviations.md
  • test/uts/objects/unit/path_object_subscribe.test.ts

@sacOO7 sacOO7 force-pushed the fix/uts-liveobjects-unit-tests branch from 6f432bf to 712b37a Compare July 10, 2026 08:24
@github-actions github-actions Bot temporarily deployed to staging/pull/2257/features July 10, 2026 08:25 Inactive
@github-actions github-actions Bot temporarily deployed to staging/pull/2257/bundle-report July 10, 2026 08:25 Inactive
@github-actions github-actions Bot temporarily deployed to staging/pull/2257/typedoc July 10, 2026 08:26 Inactive
@github-actions github-actions Bot temporarily deployed to staging/pull/2257/features July 10, 2026 09:21 Inactive
@github-actions github-actions Bot temporarily deployed to staging/pull/2257/bundle-report July 10, 2026 09:21 Inactive
@github-actions github-actions Bot temporarily deployed to staging/pull/2257/typedoc July 10, 2026 09:21 Inactive
@sacOO7 sacOO7 requested a review from Copilot July 10, 2026 09:24

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Aligns the ably-js LiveObjects unit test suite with reconciled UTS specs (notably around channel lifecycle, access preconditions, depth semantics, and quiescence controls), and closes several audit findings where assertions were previously missing or vacuous. Also includes a small runtime fix to ensure the root object is never removed from the objects pool during sync cleanup.

Changes:

  • Reconciles multiple objects/unit specs by updating scenarios (DETACHED/FAILED handling), tightening assertions (mid-sync state, noop signaling), and adding quiescence controls for negative assertions.
  • Introduces canonical test constants/helpers (SITE_CODE, ackSerial, remoteSerial, etc.) and updates tests to use sort-safe serials consistently.
  • Fixes ObjectsPool.deleteExtraObjectIds() to never delete the root object during sync reconciliation.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
test/uts/objects/unit/value_types.test.ts Increases per-test timeout to avoid flakiness in a table-driven publish/ACK round-trip test.
test/uts/objects/unit/realtime_object.test.ts Updates DETACHED/FAILED get() behavior, strengthens sync-wait assertions, revises access-precondition tests, and standardizes serial/siteCode usage.
test/uts/objects/unit/path_object.test.ts Aligns UTS tag/test naming and channel identifiers for compact() cycle detection coverage.
test/uts/objects/unit/path_object_subscribe.test.ts Reconciles depth/candidate-path semantics, adds quiescence controls, and un-skips tests where behavior is now asserted as implemented.
test/uts/objects/unit/path_object_mutations.test.ts Restores missing statusCode == 400 assertions for rejection paths.
test/uts/objects/unit/parent_references.test.ts Updates expected counter state payloads to match revised spec-derived fixtures.
test/uts/objects/unit/objects_pool.test.ts Fixes RTO3/RTO4b coverage gaps (zero-value root, non-vacuous update assertions) and updates counter fixture expectations.
test/uts/objects/unit/object_id.test.ts Aligns UTS tags/Test IDs with spec naming for objectId format/encoding checks.
test/uts/objects/unit/live_object_subscribe.test.ts Reworks noop-suppression verification to use a genuine noop path plus a positive-control dispatch.
test/uts/objects/unit/live_map.test.ts Restores noop assertions, updates spec-point commentary, and reconciles MAP_CLEAR expectations.
test/uts/objects/unit/live_map_api.test.ts Clarifies RTLM12/RTLM24 notes and makes the clear() placeholder explicitly untagged pending spec coverage.
test/uts/objects/unit/live_counter.test.ts Adds noop assertions via captureNotifyUpdated for already-merged create operations.
test/uts/objects/unit/instance.test.ts Updates serial usage and adds quiescence controls for unsubscribe/no-fire assertions; strengthens identity assertion source.
test/uts/objects/helpers/standard_test_pool.ts Adds canonical constants/helpers (SITE_CODE, ackSerial, remoteSerial, belowAckSerial) and applies them in channel setup ACK handling.
test/uts/deviations.md Updates deviation inventory to reflect reconciled specs, newly identified gaps, and newly tracked skips/adaptations.
src/plugins/liveobjects/objectspool.ts Prevents sync cleanup from deleting the root object ID from the pool.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread test/uts/objects/unit/objects_pool.test.ts Outdated
Comment thread test/uts/objects/unit/objects_pool.test.ts Outdated
@github-actions github-actions Bot temporarily deployed to staging/pull/2257/features July 10, 2026 10:07 Inactive
@github-actions github-actions Bot temporarily deployed to staging/pull/2257/bundle-report July 10, 2026 10:07 Inactive
@github-actions github-actions Bot temporarily deployed to staging/pull/2257/typedoc July 10, 2026 10:07 Inactive
…nd close audit gaps

Squash of the five stages of this PR (#2257):

1. Align objects unit tests with the reconciled UTS specs and close
   audit gaps: DETACHED/FAILED channel-lifecycle scenarios (RTO23e re-
   attach, RTO25b client-side detach, RTO20e1 ERROR->FAILED stimulus),
   restored missing/vacuous assertions (statusCode 400 rejections,
   generator-consuming [...root.keys()] precondition checks, RTO3/RTO4b
   zero-value root and update-diff assertions), quiescence controls for
   negative assertions, and UTS tag/Test-ID alignment.

2. Sync unit tests with the corrected spec fixtures and canonical serial
   helpers: SITE_CODE / ackSerial / remoteSerial / belowAckSerial
   replace hand-rolled sort-unsafe "t:N" literals; counter fixtures use
   residual count 0 with the initial value on the createOp (spec issue
   SI-1) so sync materialises count + createOp.count exactly once.

3. fix(liveobjects): never remove root from the pool during sync
   (RTO5c2a): ObjectsPool.deleteExtraObjectIds now excludes
   ROOT_OBJECT_ID, so a completed sync that omits root no longer kills
   root subscriptions. Un-skips the three path_object_subscribe tests
   this bug (and stale deviation notes) kept gated: RTPO19e2 (sync-
   triggered updates dispatch with message omitted), RTO24b2a (map-key
   candidate paths), RTO24b2b (once per dispatch).

4. deviations.md: remove entries fixed by the above (path_object_
   subscribe trio, RTINS16e2); record RTO18d under "Spec Points Under
   Review"; document four previously comment-only deviations (RTLC9/
   RTLC16 missing-field counter ops applied instead of noop; the
   RTLO4a/RTLC7d3/RTLM9b/RTLM15d4 throw-instead-of-false family;
   Instance id as getter; RTLC12e1 null increment defaulting to 1) and
   correct the stale RTLM12 keys()-via-instance() justification.

5. Review fixes: shared pollUntil helper in test/uts/helpers.ts (unit-
   tier equivalent of the spec's poll_until; flushAsync-hop interval,
   throws on timeout, matching the integration pollUntil and ably-java)
   replacing 21 hand-rolled condition polls; this.timeout(6000) on the
   objects suites whose quiescence waits exceed mocha's --no-config 2s
   default; captureNotifyUpdated shared helper with restore(); public
   size() instead of private _dataRef reach-in.

Full UTS unit suite: 1350 passing / 0 failing / 57 pending.
@sacOO7 sacOO7 force-pushed the fix/uts-liveobjects-unit-tests branch from 2d115e9 to 98ca67b Compare July 10, 2026 10:13
@sacOO7 sacOO7 merged commit a35b2a7 into uts-liveobjects Jul 10, 2026
10 of 15 checks passed
@sacOO7 sacOO7 deleted the fix/uts-liveobjects-unit-tests branch July 10, 2026 10:14
sacOO7 added a commit to ably/specification that referenced this pull request Jul 10, 2026
…specs

Implementing the reconciled objects specs against ably-js surfaced a
serial-ordering defect: the canonical ack_serial value "ack-{msgSerial}:{i}"
and the "s:1"/"s:2" serials in the RTO24a/RTO24c1 tests sort BEFORE the
standard pool's "t:0" entry timeserials under the string LWW comparison
(RTLM9), so a strictly-derived SDK rejects those operations as stale -
every local apply-on-ACK MAP_SET on a pool entry fails, and the RTO24c1
root MAP_SET never fires its subscription.

- ack_serial redefined to the sort-safe "t:" + (msgSerial + 1) + ":" + i
  form, with a NOTE documenting the ordering constraint.
- All 8 inline auto-ACK harness copies now call ack_serial(msg.msgSerial, i)
  instead of duplicating the literal, per the helper's own "never hardcode"
  convention; the 3 literal "ack-0:0" uses in the echo-dedup and
  ack-after-echo tests reference ack_serial(0, 0).
- RTO24a/RTO24c1 serials "s:1"/"s:2" changed to "t:1"/"t:2"; the adjacent
  rationale comments rewritten to state both the site-level newness and
  entry-level LWW ordering facts.
- Removed the stale note-only Test IDs RTLC12b/increment-requires-publish-0
  and RTLM20d/echo-messages-false-0 (their sections carry no steps or
  assertions - the preconditions live in realtime_object.md RTO26) so tag
  sweeps stop reporting them as permanently missing tests.
- Added the missing RTLCV3c/no-validation-at-create-0 section to
  value_types.md (no validation at blueprint creation; rejection happens
  at evaluation per RTLCV4a).

Validated by the derived ably-js suite (ably/ably-js#2257): 309 passing,
0 failing with these values; the original values reproduce 7 failures.
sacOO7 added a commit to ably/specification that referenced this pull request Jul 10, 2026
…specs

Implementing the reconciled objects specs against ably-js surfaced a
serial-ordering defect: the canonical ack_serial value "ack-{msgSerial}:{i}"
and the "s:1"/"s:2" serials in the RTO24a/RTO24c1 tests sort BEFORE the
standard pool's "t:0" entry timeserials under the string LWW comparison
(RTLM9), so a strictly-derived SDK rejects those operations as stale -
every local apply-on-ACK MAP_SET on a pool entry fails, and the RTO24c1
root MAP_SET never fires its subscription.

- ack_serial redefined to the sort-safe "t:" + (msgSerial + 1) + ":" + i
  form, with a NOTE documenting the ordering constraint.
- All 8 inline auto-ACK harness copies now call ack_serial(msg.msgSerial, i)
  instead of duplicating the literal, per the helper's own "never hardcode"
  convention; the 3 literal "ack-0:0" uses in the echo-dedup and
  ack-after-echo tests reference ack_serial(0, 0).
- RTO24a/RTO24c1 serials "s:1"/"s:2" changed to "t:1"/"t:2"; the adjacent
  rationale comments rewritten to state both the site-level newness and
  entry-level LWW ordering facts.
- Removed the stale note-only Test IDs RTLC12b/increment-requires-publish-0
  and RTLM20d/echo-messages-false-0 (their sections carry no steps or
  assertions - the preconditions live in realtime_object.md RTO26) so tag
  sweeps stop reporting them as permanently missing tests.
- Added the missing RTLCV3c/no-validation-at-create-0 section to
  value_types.md (no validation at blueprint creation; rejection happens
  at evaluation per RTLCV4a).

Validated by the derived ably-js suite (ably/ably-js#2257): 309 passing,
0 failing with these values; the original values reproduce 7 failures.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants