[ECO-5123] Fix crash when running integration tests #146
Conversation
This fixes an intermittent crash ("Non-initial ATTACHED state change
with resumed == false should have a reason") in the integration tests
which happened because the manager sometimes processed the initial
ATTACHED contributor state change after the `attach()` call completed.
I’ve followed my first suggestion in [1] for how to address this.
Resolves #121.
[1] ably/specification#239
WalkthroughThis pull request introduces significant modifications to the Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (3)
1425-1447: Address the TODO comment for specification alignmentThere's a TODO comment at line 1425 indicating that the criteria for deciding whether an
ATTACHEDstate change represents a discontinuity should be updated once aligned with the specification (see ably/specification#239). To maintain code clarity and ensure consistency with the latest specifications, consider resolving this TODO or tracking it appropriately.Would you like assistance in implementing the required changes or opening a GitHub issue to track this task?
Line range hint
1425-1480: Consider simplifying test method names for better readabilityThe test method names are quite lengthy, which might make them harder to read and maintain. For example:
func contributorAttachEvent_withResumeFalse_withOperationInProgress_withContributorAttachedPreviously_recordsPendingDiscontinuityEvent() async throwsConsider using a more concise naming convention that still conveys the purpose of the test. Shorter names can improve code readability and maintainability.
Line range hint
1478-1491: Reminder to address the TODO comment regarding specification updatesAt line 1478, there's a TODO comment about changing criteria back once aligned with the specification (see ably/specification#239). To ensure the test reflects the intended behavior per the updated specification, please consider addressing this TODO or creating a task to track it.
Would you like assistance in handling this TODO or opening a GitHub issue to ensure this gets addressed?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
Sources/AblyChat/RoomLifecycleManager.swift(4 hunks)Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift(2 hunks)
🧰 Additional context used
📓 Learnings (2)
Sources/AblyChat/RoomLifecycleManager.swift (2)
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#54
File: Sources/AblyChat/RoomLifecycleManager.swift:0-0
Timestamp: 2024-11-12T15:07:31.866Z
Learning: In `RoomLifecycleManager`, handling of unexpected contributor states after attach failure will be addressed in issue #49.
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#67
File: Sources/AblyChat/RoomLifecycleManager.swift:225-230
Timestamp: 2024-11-12T15:07:31.865Z
Learning: In the `ably-chat-swift` project, `swift-async-algorithms` provides the `async` property on collections, allowing for asynchronous operations like `contributors.async`.
Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (2)
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#54
File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:627-631
Timestamp: 2024-11-12T15:07:39.465Z
Learning: The closure `detachImpl` in `RoomLifecycleManagerTests.swift` functions as intended without external state variables to track invocation count.
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#54
File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:335-335
Timestamp: 2024-11-12T15:07:31.865Z
Learning: In Swift Testing, test method names do not need to start with `test`.
🔇 Additional comments (3)
Sources/AblyChat/RoomLifecycleManager.swift (3)
268-268: Initialization of hasBeenAttached property
The new hasBeenAttached property in ContributorAnnotation is properly initialized and integrated into the state tracking, ensuring accurate monitoring of contributor attachment states.
388-388: Addition of test utility function enhances testability
The testsOnly_subscribeToHandledContributorStateChanges() function provides a valuable tool for testing, following naming conventions and improving the capability to monitor contributor state changes in tests.
461-463: Correctly tracking contributor attachment state
Capturing the previous state with hadAlreadyAttached before updating hasBeenAttached ensures that the logic accurately reflects whether the contributor has been attached before. This is crucial for handling state changes appropriately.
Note: This is based on top of #131; please review that one first.
This fixes an intermittent crash (
Non-initial ATTACHED state change with resumed == false should have a reason) in the integration tests which happened because the manager sometimes processed the initialATTACHEDcontributor state change after theattach()call completed. I’ve followed my first suggestion in ably/specification#239 for how to address this.Resolves #121.
Summary by CodeRabbit
New Features
asyncfunctionality for room lifecycle management methods.Bug Fixes
Tests