Start implementing room lifecycle spec

This is based on [1] at b4a495e. It’s generated some questions, which I’ve asked on that PR. Note that the spec has been through a few revisions since I started implementing this; I’ve tried to keep everything in sync but some older stuff might still be accidentally there. I’ve implemented most of the specified behaviour for the ATTACH, DETACH, and RELEASE operations. I have not yet implemented RETRY since it’s quite different to those three and I wanted to get eyes on this first chunk of work; have created #51 for implementing it. Of the operations that I have implemented, I have not implemented the following (this is accurately reflected by the @SPEC… tags in the tests): - Lifecycle behaviour that relates to another ongoing operation (created #52) - Lifecycle behaviour relating to “transient disconnect timeouts” (created #48) - Lifecycle behaviour that occurs “asynchronously” outside an operation (created #50) - CHA-RL1g2, which refers to emitting “discontinuity events” which are a concept not yet implemented; this will come in #53. The room lifecycle manager introduced by this commit is currently a standalone object, which is not integrated with the rest of the SDK. I’ve created #47 for doing this integration work. The @SPEC… tags are based on the on the JS rules [2] @ 8c9ce8b, but I camel-cased them and also decided that: - if a test doesn't relate to a spec point, it doesn't need any markers - there should be a way to know that a spec point is tested across multiple tests - there should be a way of marking a spec point as implemented but not tested (so that can show up differently in reporting; important given that we’re also going to use this report as a to-do list of what still needs to be implemented) Part of #28. [1] ably/specification#200 [2] https://github.com/ably/ably-js/blob/main/CONTRIBUTING.md#tests-alignment-with-the-ably-features-specification
ably · Sep 30, 2024 · cffc6bf · cffc6bf
1 parent af41348
commit cffc6bf
Show file tree

Hide file tree

Showing 9 changed files with 1,250 additions and 5 deletions.
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
@@ -28,6 +28,7 @@ To check formatting and code quality, run `swift run BuildTool lint`. Run with `
 - We should aim to make it easy for consumers of the SDK to be able to mock out the SDK in the tests for their own code. A couple of things that will aid with this:
   - Describe the SDK’s functionality via protocols (when doing so would still be sufficiently idiomatic to Swift).
   - When defining a `struct` that is emitted by the public API of the library, make sure to define a public memberwise initializer so that users can create one to be emitted by their mocks. (There is no way to make Swift’s autogenerated memberwise initializer public, so you will need to write one yourself. In Xcode, you can do this by clicking at the start of the type declaration and doing Editor → Refactor → Generate Memberwise Initializer.)
+- When writing code that implements behaviour specified by the Chat SDK features spec, add a comment that references the identifier of the relevant spec item.
 
 ### Testing guidelines
 
@@ -51,3 +52,49 @@ A couple of notes:
 
 - Using a naming convention will allow us to verify that APIs marked `testsOnly` are not being used inside the SDK; we’ll do this in #70.
 - I believe that we should be able to eliminate the boilerplate of re-exposing a `private` member as a `testsOnly` member (as exemplified above) using a macro (called e.g. `@ExposedToTests`), but my level of experience with macros is insufficient to be confident about being able to do this quickly, so have deferred it to #71.
+
+#### Attributing tests to a spec point
+
+When writing a test that relates to a spec point from the Chat SDK features spec, add a comment that contains one of the following tags:
+
+- `@spec <spec-item-id>` — The test case directly tests all the functionality documented in the spec item.
+- `@specOneOf(m/n) <spec-item-id>` — The test case is the m<sup>th</sup> of n test cases which, together, test all the functionality documented in the spec item.
+- `@specPartial <spec-item-id>` — The test case tests some, but not all, of the functionality documented in the spec item. This is different to `@specOneOf` in that it implies that the test suite does not fully test this spec item.
+
+The `<spec-item-id>` parameter should be a spec item identifier such as `CHA-RL3g`.
+
+Each of the above tags can optionally be followed by a hyphen and an explanation of how the test relates to the given spec item.
+
+Examples:
+
+```swift
+// @spec CHA-EX3f
+func test1 { … }
+```
+
+```swift
+// @specOneOf(1/2) CHA-EX2h — Tests the case where the room is FAILED
+func test2 { … }
+
+// @specOneOf(2/2) CHA-EX2h — Tests the case where the room is SUSPENDED
+func test3 { … }
+```
+
+```swift
+// @specPartial CHA-EX1h4 - Tests that we retry, but not the retry attempt limit because we’ve not implemented it yet
+func test4 { … }
+```
+
+In [#46](https://github.com/ably-labs/ably-chat-swift/issues/46), we’ll write a script that uses these tags to generate a report about how much of the feature spec we’ve implemented.
+
+#### Marking a spec point as untested
+
+In addition to the above, you can add the following as a comment anywhere in the test suite:
+
+- `@specUntested <spec-item-id> - <explanation>` — This indicates that the SDK implements the given spec point, but that there are no automated tests for it. This should be used sparingly; only use it when there is no way to test a spec point. It must be accompanied by an explanation of why this spec point is not tested.
+
+Example:
+
+```swift
+// @specUntested CHA-EX2b - I was unable to find a way to test this spec point in an environment in which concurrency is being used; there is no obvious moment at which to stop observing the emitted state changes in order to be sure that FAILED has not been emitted twice.
+```
diff --git a/Sources/AblyChat/Errors.swift b/Sources/AblyChat/Errors.swift
@@ -16,37 +16,163 @@ public enum ErrorCode: Int {
     /// TODO this code is a guess, revisit in https://github.com/ably-labs/ably-chat-swift/issues/32
     case inconsistentRoomOptions = 1
 
+    case messagesAttachmentFailed = 102_001
+    case presenceAttachmentFailed = 102_002
+    case reactionsAttachmentFailed = 102_003
+    case occupancyAttachmentFailed = 102_004
+    case typingAttachmentFailed = 102_005
+
+    case messagesDetachmentFailed = 102_050
+    case presenceDetachmentFailed = 102_051
+    case reactionsDetachmentFailed = 102_052
+    case occupancyDetachmentFailed = 102_053
+    case typingDetachmentFailed = 102_054
+
+    case roomInFailedState = 102_101
+    case roomIsReleasing = 102_102
+    case roomIsReleased = 102_103
+
     /// The ``ARTErrorInfo.statusCode`` that should be returned for this error.
     internal var statusCode: Int {
-        // TODO: These are currently a guess, revisit in https://github.com/ably-labs/ably-chat-swift/issues/32
+        // TODO: These are currently a guess, revisit once outstanding spec question re status codes is answered (https://github.com/ably/specification/pull/200#discussion_r1755222945), and also revisit in https://github.com/ably-labs/ably-chat-swift/issues/32
         switch self {
-        case .inconsistentRoomOptions:
+        case .inconsistentRoomOptions,
+             .messagesDetachmentFailed,
+             .presenceDetachmentFailed,
+             .reactionsDetachmentFailed,
+             .occupancyDetachmentFailed,
+             .typingDetachmentFailed,
+             .roomInFailedState,
+             .roomIsReleasing,
+             .roomIsReleased:
             400
+        case .messagesAttachmentFailed,
+             .presenceAttachmentFailed,
+             .reactionsAttachmentFailed,
+             .occupancyAttachmentFailed,
+             .typingAttachmentFailed:
+            // TODO: This is currently a best guess based on the limited status code information given in the spec at time of writing (i.e. CHA-RL1h4); it's not clear to me whether these error codes are always meant to have the same status code. Revisit once aforementioned spec question re status codes answered.
+            500
         }
     }
 }
 
 /**
  The errors thrown by the Chat SDK.
 
- This type exists in addition to ``ErrorCode`` to allow us to attach metadata which can be incorporated into the error’s `localizedDescription`.
+ This type exists in addition to ``ErrorCode`` to allow us to attach metadata which can be incorporated into the error’s `localizedDescription` and `cause`.
  */
 internal enum ChatError {
     case inconsistentRoomOptions(requested: RoomOptions, existing: RoomOptions)
+    case attachmentFailed(feature: RoomFeature, underlyingError: ARTErrorInfo)
+    case detachmentFailed(feature: RoomFeature, underlyingError: ARTErrorInfo)
+    case roomInFailedState
+    case roomIsReleasing
+    case roomIsReleased
 
     /// The ``ARTErrorInfo.code`` that should be returned for this error.
     internal var code: ErrorCode {
         switch self {
         case .inconsistentRoomOptions:
             .inconsistentRoomOptions
+        case let .attachmentFailed(feature, _):
+            switch feature {
+            case .messages:
+                .messagesAttachmentFailed
+            case .occupancy:
+                .occupancyAttachmentFailed
+            case .presence:
+                .presenceAttachmentFailed
+            case .reactions:
+                .reactionsAttachmentFailed
+            case .typing:
+                .typingAttachmentFailed
+            }
+        case let .detachmentFailed(feature, _):
+            switch feature {
+            case .messages:
+                .messagesDetachmentFailed
+            case .occupancy:
+                .occupancyDetachmentFailed
+            case .presence:
+                .presenceDetachmentFailed
+            case .reactions:
+                .reactionsDetachmentFailed
+            case .typing:
+                .typingDetachmentFailed
+            }
+        case .roomInFailedState:
+            .roomInFailedState
+        case .roomIsReleasing:
+            .roomIsReleasing
+        case .roomIsReleased:
+            .roomIsReleased
+        }
+    }
+
+    /// A helper type for parameterising the construction of error messages.
+    private enum AttachOrDetach {
+        case attach
+        case detach
+    }
+
+    private static func localizedDescription(
+        forFailureOfOperation operation: AttachOrDetach,
+        feature: RoomFeature
+    ) -> String {
+        let featureDescription = switch feature {
+        case .messages:
+            "messages"
+        case .occupancy:
+            "occupancy"
+        case .presence:
+            "presence"
+        case .reactions:
+            "reactions"
+        case .typing:
+            "typing"
         }
+
+        let operationDescription = switch operation {
+        case .attach:
+            "attach"
+        case .detach:
+            "detach"
+        }
+
+        return "The \(featureDescription) feature failed to \(operationDescription)."
     }
 
     /// The ``ARTErrorInfo.localizedDescription`` that should be returned for this error.
     internal var localizedDescription: String {
         switch self {
         case let .inconsistentRoomOptions(requested, existing):
             "Rooms.get(roomID:options:) was called with a different set of room options than was used on a previous call. You must first release the existing room instance using Rooms.release(roomID:). Requested options: \(requested), existing options: \(existing)"
+        case let .attachmentFailed(feature, _):
+            Self.localizedDescription(forFailureOfOperation: .attach, feature: feature)
+        case let .detachmentFailed(feature, _):
+            Self.localizedDescription(forFailureOfOperation: .detach, feature: feature)
+        case .roomInFailedState:
+            "Cannot perform operation because the room is in a failed state."
+        case .roomIsReleasing:
+            "Cannot perform operation because the room is in a releasing state."
+        case .roomIsReleased:
+            "Cannot perform operation because the room is in a released state."
+        }
+    }
+
+    /// The ``ARTErrorInfo.cause`` that should be returned for this error.
+    internal var cause: ARTErrorInfo? {
+        switch self {
+        case let .attachmentFailed(_, underlyingError):
+            underlyingError
+        case let .detachmentFailed(_, underlyingError):
+            underlyingError
+        case .inconsistentRoomOptions,
+             .roomInFailedState,
+             .roomIsReleasing,
+             .roomIsReleased:
+            nil
         }
     }
 }
@@ -58,6 +184,11 @@ internal extension ARTErrorInfo {
         userInfo["ARTErrorInfoStatusCode"] = chatError.code.statusCode
         userInfo[NSLocalizedDescriptionKey] = chatError.localizedDescription
 
+        // TODO: This is kind of an implementation detail (that NSUnderlyingErrorKey is what populates `cause`); consider documenting in ably-cocoa as part of https://github.com/ably-labs/ably-chat-swift/issues/32.
+        if let cause = chatError.cause {
+            userInfo[NSUnderlyingErrorKey] = cause
+        }
+
         self.init(
             domain: errorDomain,
             code: chatError.code.rawValue,

diff --git a/Sources/AblyChat/RoomFeature.swift b/Sources/AblyChat/RoomFeature.swift
@@ -0,0 +1,8 @@
+/// The features offered by a chat room.
+internal enum RoomFeature {
+    case messages
+    case presence
+    case reactions
+    case occupancy
+    case typing
+}