[ECO-5397] Fix hanging typing tests

[lawrence-forooghian]

I'm still noticing an occasional hang in the integration tests, but I think that the majority of our test hangs were coming from the typing tests, so I'm re-enabling the SPM test CI step in order to be sure that these typing tests (which caused that CI step to hang) are fixed. Let's keep an eye on the integration tests.

(It's good to have resolved most of the swift test hangs, because they were also limiting the utility of Claude Code, which kept hanging when trying to run the tests to verify its changes!)

I'm also still observing the iOS unit tests taking substantially longer to execute in CI than for other platforms. For example, the result of one test run (with the integration tests disabled):

✔ Test run with 206 tests in 28 suites passed after 107.794 seconds.

where for other platforms the unit test execution time is below 1 second.

It looks like this is already covered by open issue #143, though.

Relates to #295.

Summary by CodeRabbit

• Tests

  • Re-enabled Swift tests in the automated build workflow for continuous validation
  • Enhanced test reliability by improving synchronization mechanisms for timing-dependent operations

• Refactor

  • Refined internal timing mechanisms to use deadline-based scheduling, improving precision and control flow in timer operations

[coderabbitai]

Walkthrough

Refactors sleep mechanism from duration-based to deadline-based across ClockProtocol, SystemClock, and TimerManager to improve test determinism. Updates test infrastructure with AsyncSemaphore synchronization and re-enables Swift tests in CI workflow.

Changes

Cohort / File(s)	Summary
Clock protocol & system implementation Sources/AblyChat/ClockProtocol.swift	Renames sleep(for duration:) to sleep(until deadline:) in both ClockProtocol and SystemClock. Adds Comparable conformance to ClockInstant protocol. Introduces timeInterval(since:) helper method in SystemInstant to compute duration from deadline.
Timer scheduling Sources/AblyChat/TimerManager.swift	Precomputes deadline upfront from interval and implements deadline-based wait logic. Adds early-exit optimization when current time is past deadline. Replaces duration-based sleep with deadline-based approach.
Test infrastructure Tests/AblyChatTests/Mocks/MockTestClock.swift	Updates MockTestClock method signature from sleep(for: SwiftTestDuration) to sleep(until: SwiftTestInstant). Forwards to test clock's deadline-based API instead of duration-based.
Test synchronization Tests/AblyChatTests/TypingTimerManagerTests.swift	Adds AsyncSemaphore import and synchronization to three tests. Signals semaphore inside handler and waits after clock advancement to ensure deterministic handler completion before assertions.
CI workflow .github/workflows/check.yaml	Enables previously disabled Swift tests by uncommenting swift test -Xswiftc -warnings-as-errors in SPM build-and-test step.

Sequence Diagram

sequenceDiagram
    participant Test
    participant TimerManager
    participant Clock
    participant Handler

    rect rgb(200, 240, 220)
    Note over Test,Handler: New: Deadline-Based Approach
    Test->>TimerManager: scheduleHandler(afterInterval)
    TimerManager->>TimerManager: deadline = now + interval
    TimerManager->>Clock: sleep(until: deadline)
    alt deadline already passed
        Clock->>Handler: invoke immediately
    else deadline in future
        Clock->>Clock: sleep(deadline - now)
        Clock->>Handler: invoke after sleep completes
    end
    Handler->>Test: signals AsyncSemaphore
    Test->>Test: waits for semaphore before asserting
    end

    rect rgb(220, 230, 240)
    Note over Test,Handler: Previous: Duration-Based Approach (replaced)
    Note over Test,Handler: ⚠️ No early-exit for past deadlines
    Note over Test,Handler: ⚠️ Race conditions in tests without sync
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

The refactoring follows a consistent pattern across multiple files (duration → deadline), but the changes affect critical timing semantics requiring careful verification. Logic density is moderate with precomputation and early-exit optimization in TimerManager, and test synchronization patterns require validation for correctness and completeness.

Possibly related issues

• Tests seem to be intermittently hanging in CI #295 – The shift from duration-based to deadline-based sleep in ClockProtocol, SystemClock, and TimerManager, combined with AsyncSemaphore synchronization in tests, directly addresses the intermittent test-hanging behavior reported in the linked issue ECO-5397 by eliminating race conditions in timing-dependent tests.

Poem

🐰 Hop, hop—the clock now counts toward deadlines ahead,
No more guessing durations, just knowing where we're led.
Semaphores stand ready, synchronized and true,
Tests race no longer; hangs bid swift adieu! 🎉

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The PR title "ECO-5397 Fix hanging typing tests" directly aligns with the primary objective of the changeset, which is to address intermittent CI test hangs specifically related to typing tests. The title is concise and specific, clearly conveying the main purpose of the changes without unnecessary noise. A teammate scanning the commit history would immediately understand that this PR addresses a test reliability issue related to typing functionality.
Linked Issues Check	✅ Passed	The code changes comprehensively address the objectives outlined in linked issue ECO-5397. The PR investigates and implements fixes for CI test hangs by converting the sleep mechanism from duration-based to deadline-based across ClockProtocol, SystemClock, TimerManager, and MockTestClock ECO-5397. Additionally, AsyncSemaphore synchronization is introduced in TypingTimerManagerTests to ensure deterministic test execution and prevent hanging, while the SPM test CI step is re-enabled to verify the fixes ECO-5397. These changes directly target the root causes of the intermittent typing test hangs mentioned in the issue.
Out of Scope Changes Check	✅ Passed	All changes in the PR are directly related to fixing the hanging typing tests objective from ECO-5397. The architectural shift from duration-based to deadline-based sleep in ClockProtocol and related implementations targets the root cause of the hangs, while the addition of AsyncSemaphore synchronization in TypingTimerManagerTests ensures deterministic test execution. The re-enabling of the SPM test CI step and updates to MockTestClock support these fixes without introducing unrelated functionality. No out-of-scope changes are present.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch 295-fix-hanging-typing-tests

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between a131892 and de65183.

📒 Files selected for processing (5)

• .github/workflows/check.yaml (1 hunks)
• Sources/AblyChat/ClockProtocol.swift (3 hunks)
• Sources/AblyChat/TimerManager.swift (1 hunks)
• Tests/AblyChatTests/Mocks/MockTestClock.swift (1 hunks)
• Tests/AblyChatTests/TypingTimerManagerTests.swift (4 hunks)

🧰 Additional context used

📓 Path-based instructions (4)

**/*.swift

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.swift: Use protocol-based design; expose SDK functionality via protocols and prefer associated types with opaque return types (some Protocol) instead of existentials (any Protocol)
Isolate all mutable state to the main actor; mark stateful objects with @mainactor
Public API must use typed throws with ErrorInfo; use InternalError internally and convert at the public API boundary
For public structs emitted by the API, provide an explicit public memberwise initializer
When using AsyncSequence operators in @mainactor contexts, mark operator closures as @sendable
Task, CheckedContinuation, and AsyncThrowingStream do not support typed errors; use Result and call .get() to surface typed errors
Do not use Dictionary.mapValues for typed throws; use ablyChat_mapValuesWithTypedThrow instead
When the compiler struggles with typed throws, explicitly declare the error type on do blocks (e.g., do throws(InternalError))
Specify error types in closures using the throws(ErrorType) syntax (e.g., try items.map { jsonValue throws(InternalError) in ... })
Mark any test-only APIs with testsOnly_ prefix and wrap them in #if DEBUG

Files:

• Sources/AblyChat/TimerManager.swift
• Sources/AblyChat/ClockProtocol.swift
• Tests/AblyChatTests/Mocks/MockTestClock.swift
• Tests/AblyChatTests/TypingTimerManagerTests.swift

Tests/AblyChatTests/**/*.swift

📄 CodeRabbit inference engine (CLAUDE.md)

Tests/AblyChatTests/**/*.swift: Use test attribution tags to reference spec coverage: @SPEC, @specOneOf(m/n), @specPartial, @specUntested, @specNotApplicable
For Swift Testing #expect(throws:) with typed errors, move typed-throw code into a separate non-typed-throw function until Xcode 16.3+

Files:

• Tests/AblyChatTests/Mocks/MockTestClock.swift
• Tests/AblyChatTests/TypingTimerManagerTests.swift

Tests/AblyChatTests/**

📄 CodeRabbit inference engine (CLAUDE.md)

Place tests under Tests/AblyChatTests/ as the root for test targets

Files:

• Tests/AblyChatTests/Mocks/MockTestClock.swift
• Tests/AblyChatTests/TypingTimerManagerTests.swift

Tests/AblyChatTests/Mocks/**/*.swift

📄 CodeRabbit inference engine (CLAUDE.md)

Put mock implementations under Tests/AblyChatTests/Mocks/

Files:

• Tests/AblyChatTests/Mocks/MockTestClock.swift

🧬 Code graph analysis (4)

Sources/AblyChat/TimerManager.swift (2)

Sources/AblyChat/ClockProtocol.swift (2)

• advanced (48-50)
• sleep (20-25)

Tests/AblyChatTests/Mocks/MockTestClock.swift (2)

• advanced (43-45)
• sleep (17-19)

Sources/AblyChat/ClockProtocol.swift (1)

Tests/AblyChatTests/Mocks/MockTestClock.swift (1)

• sleep (17-19)

Tests/AblyChatTests/Mocks/MockTestClock.swift (1)

Sources/AblyChat/ClockProtocol.swift (1)

• sleep (20-25)

Tests/AblyChatTests/TypingTimerManagerTests.swift (2)

Sources/AblyChat/TypingTimerManager.swift (1)

• startTypingTimer (44-64)

Tests/AblyChatTests/Mocks/MockTestClock.swift (1)

• advance (21-23)

🔇 Additional comments (12)

.github/workflows/check.yaml (1)

95-95: SPM tests re-enabled successfully.

The re-enablement of the Swift Package Manager test step aligns with the PR objective of fixing hanging typing tests. Given the PR description mentions "occasional remaining hangs in integration tests," monitor CI runs closely to ensure stability.

Tests/AblyChatTests/TypingTimerManagerTests.swift (3)

4-4: LGTM! Semaphore import enables test synchronization.

The import of the Semaphore library provides AsyncSemaphore, which is essential for coordinating asynchronous timer handler execution in tests under the new deadline-based timing model.

---

87-108: Excellent synchronization pattern for deadline-based timing.

The AsyncSemaphore usage correctly ensures the timer handler completes before assertions run. The pattern of signal-in-handler and wait-before-assert prevents race conditions that could cause test flakiness with the deadline-based sleep model.

---

150-174: Consistent synchronization pattern applied.

The AsyncSemaphore usage mirrors the pattern in typingTimerExpiration, ensuring deterministic test behavior when validating grace period timing.

Sources/AblyChat/TimerManager.swift (3)

15-16: Excellent design decision for test determinism.

Precomputing the deadline before launching the Task ensures that TestClock.advance(by:) behaves reliably regardless of Task scheduling delays. This addresses the root cause of the hanging tests by guaranteeing the sleep call responds to the correct clock time reference.

---

19-25: Proper edge case handling for TestClock compatibility.

The early-exit optimization correctly handles the scenario where Task scheduling delays cause the deadline to be in the past. This prevents unexpected blocking behavior and ensures timers fire immediately when appropriate, addressing TestClock issue #23.

---

27-32: Core deadline-based sleep implementation is correct.

The transition from duration-based to deadline-based sleep, combined with the subsequent cancellation check, correctly implements the timer behavior. Using try? appropriately suppresses errors while the cancellation check ensures proper cleanup.

Tests/AblyChatTests/Mocks/MockTestClock.swift (1)

17-18: API migration to deadline-based sleep is correct.

The signature change from sleep(for:) to sleep(until:) properly aligns MockTestClock with the updated ClockProtocol. The delegation to the underlying TestClock maintains the adapter pattern correctly.

Sources/AblyChat/ClockProtocol.swift (4)

9-9: Protocol redesign improves test determinism.

The shift from duration-based sleep(for:) to deadline-based sleep(until:) is a sound architectural decision. It enables more reliable test clock behavior and eliminates the timing ambiguities that caused the hanging tests.

---

20-25: SystemClock implementation correctly handles deadline semantics.

The implementation properly computes the remaining duration from the deadline and guards against negative sleep durations. This ensures production code behaves correctly when deadlines are in the past.

---

29-29: Comparable conformance enables deadline comparisons.

Adding Comparable to ClockInstant is necessary to support the clock.now >= deadline check in TimerManager. The existing comparison operator implementations satisfy this requirement.

---

52-54: Helper method encapsulates time interval computation.

The timeInterval(since:) method provides a clean abstraction for computing durations between instants, which is essential for the deadline-based sleep implementation in SystemClock.

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[maratal]

I feel like each test should have begin/end timestamp. If this will not make it obvious what test(s) exactly causes hang, then we can try other things, wdyt?

[lawrence-forooghian]

I feel like each test should have begin/end timestamp. If this will not make it obvious what test(s) exactly causes hang, then we can try other things, wdyt?

Could be interesting — could you have a think about how we'd achieve that? I'd suggest looking into Swift Testing's test hooks functionality (they don't call it that, forgot the term they use). But I don't think it should stop us from merging this?