Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 59 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (55)
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. Comment |
VeskeR
force-pushed
the
AIT-1378/uts-realtime-unit
branch
from
October 2, 2026 00:31
c2b2aea to
d1d327f
Compare
VeskeR
force-pushed
the
AIT-1378/uts-unit
branch
from
October 2, 2026 00:31
99b43c9 to
d065c61
Compare
This was referenced Oct 2, 2026
VeskeR
changed the title
This was referenced Oct 2, 2026
VeskeR
force-pushed
the
AIT-1378/uts-unit
branch
from
October 2, 2026 01:13
d065c61 to
c36c9ac
Compare
VeskeR
force-pushed
the
AIT-1378/uts-realtime-unit
branch
from
October 2, 2026 01:13
d1d327f to
fcfe8af
Compare
VeskeR
changed the title
Neither of these is new, and neither is a UTS test. Both are races that have presumably always been there and that only lose once the machine has enough else to do - which the UTS realtime tier, landing next, provides. ChannelsSpecs.ReleaseAll_ShouldRemoveChannelWhenDetached asserted the channel collection was empty immediately after the channel reached DETACHED. The release is applied on the workflow afterwards, so the assertion can run first. Its sibling one function down, ReleaseAll_ShouldRemoveChannelWhenFailed, already waits for the workflow; this one now does too. Without it the test fails on every Release run once the realtime tier is in the assembly. ConnectionAwaiter checked the current state and then subscribed, with a gap a transition can fall through - seen by neither the check nor the listener. The tell is the contradiction in its own timeout message, "expected 'Connected' but current state was 'Connected'", which is what net7.0 produced under load. It now re-checks after subscribing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Forty-eight of the fifty-four uts/realtime/unit spec files: the connection's lifecycle and its failure modes, authentication, the realtime client's own REST surface, the whole of presence, and the whole of channels. Everything runs off MockWebSocket. Measured on net6.0: 426 passing, 48 env-gated as SDK deviations. The six files not translated need API this SDK does not have - annotations, message mutation, whenState - and the next commit tabulates them. Uts/Helpers/VcdiffDeltas.cs arrives with this tier because only this tier needs it. The delta-decoding specs build their deltas with a mock encoder paired with a mock decoder installed as a plugin; there is no plugin seam here, the codec is IO.Ably.DeltaCodec compiled in, and that library decodes only. So the deltas have to be real ones, and this is a small real RFC 3284 encoder that produces them. Being real is the point: it emits a COPY for any run it finds in the source, so the delta genuinely depends on the base payload. A mock delta that ignored its source would decode identically against a right base and a wrong one, which would have made most of RTL19 and RTL20 vacuous. Three properties of this tier are worth knowing before writing another test in it. Connection.NotifyOperatingSystemNetworkState is static and every Connection subscribes to it from its own constructor whatever AutomaticNetworkStateMonitoring says, so calling it simulates a network change for every live client in the process - measured, it destabilised three of the repo's own unit tests. The tier therefore runs in a single xUnit collection: these tests share one process with real timers and no timer seam, and serialising them took eleven failures to three on its own. And FAILED is not terminal here, so a test that samples Connection.State or ErrorReason after awaiting FAILED reads a property the SDK has already cleared on its way back to CONNECTING. What the tier found, with the mechanism located for each: Automatic presence re-entry never reaches the wire after a reconnect. The machinery is correct and the internal map survives; ChannelMessageProcessor calls Presence.ChannelAttached before SetChannelState(Attached), so each re-entry is queued into a queue SendQueuedMessages flushed three lines earlier. The other call site - RTL12's, for an ATTACHED on an already-attached channel - does it the right way round and works. That is both the proof and the fix. A pending attach or detach is told it failed and then succeeds. The channel fails both awaiters on DISCONNECTED; RTN19b then resends the ATTACH on the new transport and the channel attaches. RTL4d says that callback fires when the channel next moves to ATTACHED, DETACHED, SUSPENDED or FAILED, and DISCONNECTED is none of them. The correct behaviour is implemented twenty lines above in the same switch. A message's own timestamp is always overwritten by its envelope's, and wiped to null when the envelope has none. DecodeMessages has the guarded assignment TM2f asks for; it is dead code, because ParseRealtimeData has already assigned unconditionally. Presence messages too. A FAILED state change is swallowed when a synchronous continuation throws inside an unguarded internal-handler emit. A server-initiated DETACHED emits a spurious DETACHED state change, after the ATTACHING that claims to have come from it, because SetChannelState runs its handler - which can recurse into SetChannelState - before emitting its own change. One correction to the inherited census: channel_publish.md was listed as six tests blocked on PublishResult. Re-measured, three of those six carry a substantive assertion alongside - incrementing msgSerials, a publish surviving DISCONNECTED, a message resent on a new transport - and are translated with that one line dropped and said so in the test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The realtime half of the unit tier, added to the same two documents the REST half started. Twenty-seven more SDK defects, one more UTS spec error, three more adapted tests, one more mock limitation and one more investigated suspicion - and in coverage.md the six spec files that could not be written, with the client API each one needs. Two of the defects are worth reading even if the rest are skimmed, because both are a correct implementation defeated by call order rather than missing code. Automatic presence re-entry queues its messages one line after the queue is flushed. A pending attach is failed on DISCONNECTED, which RTL4d does not list as one of the states that resolves it, and then succeeds anyway when the attach is resent. Each entry names the file and line, so the fix is a short diff rather than an investigation. The README gains what this tier needs its reader to know: that these tests run in a single xUnit collection, declared on UtsTestBase.RealtimeUnitCollection, and that a new class needs the attribute because xUnit does not inherit it. That is not tidiness - serialising them took the failure count from eleven to three. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
VeskeR
force-pushed
the
AIT-1378/uts-realtime-unit
branch
from
October 2, 2026 13:16
fcfe8af to
3e51d29
Compare
VeskeR
force-pushed
the
AIT-1378/uts-unit
branch
from
October 2, 2026 13:16
c36c9ac to
3d3864b
Compare
This branch was successfully deployed
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.
The stack
Six PRs, each based on the one before it. Review in order.
uts-to-csharptranslation skillGoal
Translate the realtime half of the UTS unit tier, and record what the translation found.
Three commits: two pre-existing races fixed first, then the tests, then the documents.
What's added
Forty-eight of the fifty-four
uts/realtime/unitspec files — the connection's lifecycle andits failure modes, authentication, the realtime client's own REST surface, the whole of presence,
and the whole of channels. Everything runs off
MockWebSocket. Measured onnet6.0: 426passing, 48 env-gated as SDK deviations.
The six files not translated need API this SDK does not have — annotations, message mutation,
whenState— and are tabulated incoverage.mdwith the reason for each.Uts/Helpers/VcdiffDeltas.csarrives with this tier because only this tier needs it. Thedelta-decoding specs build their deltas with a mock encoder paired with a mock decoder installed as
a plugin; there is no plugin seam here, the codec is
IO.Ably.DeltaCodeccompiled in, and thatlibrary decodes only. So the deltas have to be real ones, and this is a small real RFC 3284 encoder
that produces them. Being real is the point: it emits a COPY for any run it finds in the source, so
the delta genuinely depends on the base payload. A mock delta that ignored its source would decode
identically against a right base and a wrong one, which would have made most of RTL19 and RTL20
vacuous.
Two pre-existing races, fixed first
Neither is new and neither is a UTS test. Both are races that have presumably always been there and
that only lose once the machine has enough else to do — which this tier provides.
ChannelsSpecs.ReleaseAll_ShouldRemoveChannelWhenDetachedasserted the channel collection wasempty immediately after the channel reached DETACHED. The release is applied on the workflow
afterwards, so the assertion can run first. Its sibling one function down already waits for the
workflow; this one now does too. Without it the test fails on every Release run once the realtime
tier is in the assembly.
ConnectionAwaiterchecked the current state and then subscribed, with a gap a transition canfall through — seen by neither the check nor the listener. The tell is the contradiction in its
own timeout message, "expected 'Connected' but current state was 'Connected'", which is what
net7.0produced under load. It now re-checks after subscribing.What the translation found
Automatic presence re-entry never reaches the wire after a reconnect. The machinery is correct
and the internal map survives;
ChannelMessageProcessorcallsPresence.ChannelAttachedbeforeSetChannelState(Attached), so each re-entry is queued into a queueSendQueuedMessagesflushedthree lines earlier. The other call site — RTL12's, for an ATTACHED on an already-attached channel —
does it the right way round and works. That is both the proof and the fix.
A pending attach or detach is told it failed and then succeeds. The channel fails both awaiters
on DISCONNECTED; RTN19b then resends the ATTACH on the new transport and the channel attaches.
RTL4d says that callback fires when the channel next moves to ATTACHED, DETACHED, SUSPENDED or
FAILED, and DISCONNECTED is none of them. The correct behaviour is implemented twenty lines above
in the same
switch.A message's own timestamp is always overwritten by its envelope's, and wiped to
nullwhen theenvelope has none.
DecodeMessageshas the guarded assignment TM2f asks for; it is dead code,because
ParseRealtimeDatahas already assigned unconditionally. Presence messages too.A FAILED state change is swallowed when a synchronous continuation throws inside an unguarded
internal-handler emit.
A server-initiated DETACHED emits a spurious DETACHED state change, after the ATTACHING that
claims to have come from it, because
SetChannelStateruns its handler — which can recurse intoSetChannelState— before emitting its own change.Caveats
Three properties of this tier are worth knowing before writing another test in it.
Connection.NotifyOperatingSystemNetworkStateis static and everyConnectionsubscribes to itfrom its own constructor whatever
AutomaticNetworkStateMonitoringsays, so calling it simulatesa network change for every live client in the process. Measured: it destabilised three of the
repo's own unit tests.
UtsTestBase.RealtimeUnitCollection.These tests share one process with real timers and there is no timer seam, so serialising them is
not free tidiness — it took the failure count from eleven to three. A new realtime unit class
needs that
[Collection]attribute; xUnit does not inherit it from a base class.Connection.StateorErrorReasonafterawaiting FAILED reads a property the SDK has already cleared on its way back to CONNECTING.
One correction to the inherited census.
channel_publish.mdwas listed as six tests blocked onPublishResult. Re-measured, three of those six carry a substantive assertion alongside —incrementing
msgSerials, a publish surviving DISCONNECTED, a message resent on a new transport —and are translated with that one line dropped and said so in the test.
🤖 Generated with Claude Code