[ECO-5063] feat: add SDK tracking for Rest and Realtime calls #107
Conversation
WalkthroughThis pull request introduces a new dependency for the Ably Pub/Sub adapter and updates multiple components to use improved type safety. Several API methods now accept enumerated HTTP method values rather than strings, and the realtime client usage has been refactored to employ a wrapper (channelWrapper) around the underlying channel. Various classes in production and test code have been updated to utilize the new RealtimeChannel/RealtimePresence types, including changes to room attachment, lifecycle management, and presence subscription. These changes are implemented across production code and tests to improve clarity and consistency. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as MainActivity
participant CC as ChatClient
participant RT as AblyRealtime
participant WR as RealtimeClientWrapper
participant API as ChatApi
UI->>CC: Create ChatClient(realtime)
CC->>RT: Instantiate with AblyRealtime
CC->>WR: Create realtimeClientWrapper via createWrapperSdkProxy
WR->>API: Route API calls (e.g., HTTP requests with HttpMethod enum)
API->>RT: Execute authorized requests
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Pro Plan! Simply use the command ✨ Finishing Touches
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 (
|
Code Coverage
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
chat-android/src/test/java/com/ably/chat/room/RoomTestHelpers.kt (2)
26-26: Expanded mock usage
import io.mockk.everysignals more flexible mocking. Ensure that all expectations are covered in test cleanup.
121-125: Spying room feature properties
Spying onroom.messages,room.presence, etc., centralizes test logic. Beware of partial mocks that can hide code coverage or real logic.chat-android/src/test/java/com/ably/chat/room/lifecycle/AttachTest.kt (3)
45-45: Remove unused import.The import for
org.junit.Ignoreis not used in this file and should be removed.-import org.junit.Ignore🧰 Tools
🪛 GitHub Actions: Check
[warning] 45-45: Detekt: Unused import [NoUnusedImports]
401-401: Fix typo in test name.The test specification reference has a double hyphen "CHA--RL1h6" instead of a single hyphen.
-fun `(CHA--RL1h6, CHA-RC2e) When room enters failed state, when CHA-RL1h5 fails to detach, op will be repeated till all channels are detached`() = runTest { +fun `(CHA-RL1h6, CHA-RC2e) When room enters failed state, when CHA-RL1h5 fails to detach, op will be repeated till all channels are detached`() = runTest {
248-249: Consider adding a comment explaining the InternalAPI usage.The
@OptIn(InternalAPI::class)annotation suggests using internal APIs which might not be stable. Consider adding a brief comment explaining why it's necessary to use internal APIs in this test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
chat-android/build.gradle.kts(1 hunks)chat-android/src/main/java/com/ably/chat/ChatApi.kt(8 hunks)chat-android/src/main/java/com/ably/chat/ChatClient.kt(4 hunks)chat-android/src/main/java/com/ably/chat/Discontinuities.kt(1 hunks)chat-android/src/main/java/com/ably/chat/Messages.kt(6 hunks)chat-android/src/main/java/com/ably/chat/Occupancy.kt(3 hunks)chat-android/src/main/java/com/ably/chat/Presence.kt(3 hunks)chat-android/src/main/java/com/ably/chat/Room.kt(3 hunks)chat-android/src/main/java/com/ably/chat/RoomLifecycleManager.kt(7 hunks)chat-android/src/main/java/com/ably/chat/RoomReactions.kt(4 hunks)chat-android/src/main/java/com/ably/chat/Rooms.kt(1 hunks)chat-android/src/main/java/com/ably/chat/Typing.kt(6 hunks)chat-android/src/main/java/com/ably/chat/Utils.kt(8 hunks)chat-android/src/test/java/com/ably/chat/ChatApiTest.kt(1 hunks)chat-android/src/test/java/com/ably/chat/MessagesTest.kt(7 hunks)chat-android/src/test/java/com/ably/chat/OccupancyTest.kt(2 hunks)chat-android/src/test/java/com/ably/chat/PresenceTest.kt(5 hunks)chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt(2 hunks)chat-android/src/test/java/com/ably/chat/TestUtils.kt(4 hunks)chat-android/src/test/java/com/ably/chat/TypingTest.kt(2 hunks)chat-android/src/test/java/com/ably/chat/room/RoomFeatureSharedChannelTest.kt(2 hunks)chat-android/src/test/java/com/ably/chat/room/RoomTestHelpers.kt(3 hunks)chat-android/src/test/java/com/ably/chat/room/lifecycle/AttachTest.kt(13 hunks)chat-android/src/test/java/com/ably/chat/room/lifecycle/DetachTest.kt(9 hunks)chat-android/src/test/java/com/ably/chat/room/lifecycle/ReleaseTest.kt(7 hunks)chat-android/src/test/java/com/ably/chat/room/lifecycle/RetryTest.kt(8 hunks)example/src/main/java/com/ably/chat/example/MainActivity.kt(2 hunks)gradle/libs.versions.toml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- chat-android/src/main/java/com/ably/chat/Rooms.kt
- chat-android/src/test/java/com/ably/chat/ChatApiTest.kt
🧰 Additional context used
🧠 Learnings (10)
chat-android/src/main/java/com/ably/chat/Occupancy.kt (1)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/main/java/com/ably/chat/Occupancy.kt:145-145
Timestamp: 2024-11-28T11:11:20.423Z
Learning: In `chat-android/src/main/java/com/ably/chat/Occupancy.kt`, within the `DefaultOccupancy` class, when methods use `room.chatApi`, which utilizes the REST API, there's no need to call `room.ensureAttached()` before performing the operation.
chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt (1)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt:44-50
Timestamp: 2024-11-28T11:08:42.524Z
Learning: The test cases for verifying behavior when the room is not in the ATTACHED state are covered in `chat-android/src/test/java/com/ably/chat/room/RoomEnsureAttachedTest.kt`.
chat-android/src/main/java/com/ably/chat/Room.kt (1)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/main/java/com/ably/chat/Room.kt:0-0
Timestamp: 2024-11-28T11:09:34.042Z
Learning: In the `DefaultRoom` class, the `ensureAttached()` method checks `statusLifecycle.status` again inside `roomScope.launch` to handle possible state changes, ensuring robustness.
chat-android/src/test/java/com/ably/chat/OccupancyTest.kt (1)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/test/java/com/ably/chat/OccupancyTest.kt:20-31
Timestamp: 2024-11-28T11:12:06.843Z
Learning: In `OccupancyTest.kt`, additional room state initialization using `setState` is unnecessary for testing occupancy functionality, as the specification doesn't require it.
chat-android/src/test/java/com/ably/chat/room/RoomFeatureSharedChannelTest.kt (2)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#89
File: chat-android/src/test/java/com/ably/chat/room/RoomFeatureSharedChannelTest.kt:44-48
Timestamp: 2024-12-10T14:47:31.766Z
Learning: In tests in `RoomFeatureSharedChannelTest.kt` for the Ably Chat Kotlin project, the order of `ChannelMode.publish` and `ChannelMode.subscribe` in the `modes` array does not affect functionality; assertions should not enforce a specific order for these modes.
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#75
File: chat-android/src/test/java/com/ably/chat/room/RoomFeatureSharedChannelTest.kt:43-45
Timestamp: 2024-12-02T12:10:18.954Z
Learning: In the `RoomFeatureSharedChannelTest.kt` tests, prefer using assertions like `Assert.assertEquals` that provide detailed error messages over general assertions like `Assert.assertTrue`, to aid in debugging when a test fails due to a missing mode.
chat-android/src/test/java/com/ably/chat/room/lifecycle/RetryTest.kt (1)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt:44-50
Timestamp: 2024-11-28T11:08:42.524Z
Learning: The test cases for verifying behavior when the room is not in the ATTACHED state are covered in `chat-android/src/test/java/com/ably/chat/room/RoomEnsureAttachedTest.kt`.
chat-android/src/main/java/com/ably/chat/RoomReactions.kt (1)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#100
File: chat-android/src/main/java/com/ably/chat/RoomReactions.kt:156-156
Timestamp: 2025-02-14T13:27:05.487Z
Learning: The server always ensures that the metadata field in room reactions is sent as a JsonObject, eliminating the need for explicit type checking on the client side.
chat-android/src/test/java/com/ably/chat/PresenceTest.kt (1)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/test/java/com/ably/chat/PresenceTest.kt:33-33
Timestamp: 2024-11-28T11:08:38.559Z
Learning: The `RoomEnsureAttachedTest.kt` file contains tests that verify room operations are only performed when the room is in the `ATTACHED` state, including scenarios where operations fail when the room is not attached, succeed when it is attached, and proper error handling for invalid room states.
chat-android/src/test/java/com/ably/chat/room/lifecycle/AttachTest.kt (1)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt:44-50
Timestamp: 2024-11-28T11:08:42.524Z
Learning: The test cases for verifying behavior when the room is not in the ATTACHED state are covered in `chat-android/src/test/java/com/ably/chat/room/RoomEnsureAttachedTest.kt`.
chat-android/src/test/java/com/ably/chat/room/lifecycle/ReleaseTest.kt (1)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt:44-50
Timestamp: 2024-11-28T11:08:42.524Z
Learning: The test cases for verifying behavior when the room is not in the ATTACHED state are covered in `chat-android/src/test/java/com/ably/chat/room/RoomEnsureAttachedTest.kt`.
🪛 GitHub Actions: Check
chat-android/src/test/java/com/ably/chat/room/lifecycle/AttachTest.kt
[warning] 45-45: Detekt: Unused import [NoUnusedImports]
🔇 Additional comments (163)
gradle/libs.versions.toml (1)
27-27: Added Ably PubSub adapter dependency.This addition aligns with the PR objective to enhance SDK tracking by introducing the PubSub adapter, which provides improved interfaces for the Ably Java SDK.
chat-android/build.gradle.kts (1)
54-54: Implemented the PubSub adapter dependency.Correctly implements the newly added PubSub adapter library, which will enable the SDK tracking improvements described in the PR objectives.
chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt (2)
48-48: Updated test to use channelWrapper instead of realtimeChannel.The test now properly uses the
channelWrapperproperty fromroomReactionswith a relaxed mock return value, which aligns with the SDK tracking enhancements.
56-56: Updated verification to use channelWrapper.The verification logic has been updated to check interactions with
channelWrapperinstead of the previousrealtimeChannel, maintaining test consistency.chat-android/src/main/java/com/ably/chat/Occupancy.kt (3)
5-5: Added import for RealtimeChannel.Adding the import for the new Kotlin-based RealtimeChannel interface from the PubSub adapter library.
89-90: Added channelWrapper property using RealtimeChannel type.This addition implements the new Kotlin-based interface that supports the SDK tracking through the
createWrapperSDKProxymethod as mentioned in the PR objectives.
114-114: Updated subscription mechanism to use channelWrapper.The subscription now uses the channelWrapper with the asChatSubscription() extension method, which simplifies the subscription management while enabling SDK tracking.
chat-android/src/main/java/com/ably/chat/Room.kt (2)
3-3: Addition of RealtimeClient import for SDK tracking purposes.This import enables the updated interface that supports the SDK tracking features mentioned in the PR objectives.
268-280: Enhanced error handling for Attaching state.The implementation now properly manages the room attachment lifecycle by using
onChangeOnceto monitor status transitions. This provides more detailed error handling and logging when attachment fails, which aligns with the SDK tracking enhancement goals.This implementation also follows the established pattern of checking
statusLifecycle.statusagain insideroomScope.launchto handle possible state changes, ensuring robustness.example/src/main/java/com/ably/chat/example/MainActivity.kt (2)
52-52: Switching to AblyRealtime for SDK tracking capabilities.The import change from
RealtimeClienttoAblyRealtimeenables the SDK tracking functionality described in the PR objectives.
65-65: Updated client instantiation to use AblyRealtime.This change implements the interface update from
RealtimeClienttoAblyRealtimewhile maintaining the same configuration parameters, aligning with the PR objective to replace old Ably Java interfaces with new Kotlin-based ones.chat-android/src/test/java/com/ably/chat/TestUtils.kt (3)
3-4: Updated imports for HTTP method enum and RealtimeClient.These imports support the transition to more type-safe API calls using enums instead of string literals, which is part of the broader interface improvement.
26-27: Improved type safety by using HttpMethod enum.Replacing string literals ("GET", "POST") with
HttpMethod.GetandHttpMethod.Postenum values enhances type safety and aligns with the updated SDK interfaces.Also applies to: 37-38, 50-51
28-28: Updated callback argument position.Changed from
lastArg<AsyncHttpPaginatedResponse.Callback>()tosecondArg<AsyncHttpPaginatedResponse.Callback>()to accommodate the updated method signature in the new interface.Also applies to: 39-39, 52-52
chat-android/src/test/java/com/ably/chat/OccupancyTest.kt (2)
4-4: Updated imports for testing with RealtimeChannel.These imports support the updated testing approach that aligns with the new SDK interfaces.
Also applies to: 9-9
25-29: Updated mock setup for RealtimeChannel.The test now uses
createMockRealtimeChannelinstead of the previous approach, ensuring compatibility with the new Kotlin-based interfaces introduced for SDK tracking. The channel subscription is now more flexible withmockk(relaxUnitFun = true).chat-android/src/main/java/com/ably/chat/Discontinuities.kt (3)
3-3: Added import for the new RealtimeChannel type.This import reflects the migration from Ably's Java interfaces to the new Kotlin-based interfaces, as stated in the PR objectives.
10-10: Changed interface visibility to internal.Restricting the
HandlesDiscontinuityinterface visibility to internal enhances encapsulation by ensuring it's only accessible within the module.
14-14: Updated channel property to use RealtimeChannel wrapper.Changed from
val channel: AblyRealtimeChanneltoval channelWrapper: RealtimeChannel. This change:
- Adopts the new Kotlin-based interface type
- Renames the property to indicate it's now a wrapper, supporting the SDK tracking functionality
This change is consistent with the PR objective to upgrade interfaces for improved SDK tracking.
chat-android/src/test/java/com/ably/chat/room/RoomFeatureSharedChannelTest.kt (6)
21-22: Extracted channels reference to improve code readability.The channels property is now stored in a local variable, reducing repetition and improving readability in subsequent calls.
24-28: Updated channel access pattern and mock creation.The channel access pattern has been updated to use the local variable, and the mock creation method has been updated to use
createMockRealtimeChannel()with a specific channel name. This change aligns with the new SDK structure.
53-55: Updated channel verification to use local variable.Using the local
channelsvariable for verification maintains consistent usage throughout the test.
57-59: Updated typing indicators channel verification to use local variable.Consistently using the local
channelsvariable for verification.
61-63: Updated reactions channel verification to use local variable.Consistently using the local
channelsvariable for verification.
65-67: Updated general channel verification to use local variable.Consistently using the local
channelsvariable for final verification.chat-android/src/test/java/com/ably/chat/room/lifecycle/DetachTest.kt (10)
3-4: Added import for InternalAPI annotation.This import is necessary for the
@OptIn(InternalAPI::class)annotations added to test methods that use internal APIs.
19-19: Updated import to use new RealtimeChannel type.Replacing the old
io.ably.lib.realtime.Channelwith the new Kotlin-basedcom.ably.pubsub.RealtimeChannelinterface.
58-59: Updated tearDown method to unmock the correct class.Unmocking the static methods on the new
RealtimeChannelclass instead of the oldChannelclass.
150-154: Updated channel mocking for detach operations.The mock setup has been modified to:
- Mock static methods on
RealtimeChannelinstead ofChannel- Update the capturedChannels list to use the new type
- Update the coEvery block to use the new type
This change aligns the test with the new SDK structure while maintaining the same test coverage.
166-167: Updated channel access pattern in assertions.Changed from accessing
contributors[it].channel.nametocontributors[it].channelWrapper.name, aligning with the updated interface.
224-224: Added @OptIn annotation for using internal APIs.This annotation explicitly acknowledges that the test is using internal APIs, which improves code clarity and documentation.
230-248: Updated channel state handling in detach failure test.The test now:
- Mocks static methods on
RealtimeChannel- Sets state and reason properties on the channel object
- Additionally updates the Java channel for backward compatibility
This comprehensive update ensures the test continues to work with the new SDK structure.
279-279: Added @OptIn annotation for using internal APIs in another test method.Consistently applying the annotation to all test methods that use internal APIs.
291-308: Updated channel state handling in multiple failure test.Similar to the previous test, this updates the channel mocking and state handling to work with the new
RealtimeChannelinterface.
343-352: Updated detach retry test to use RealtimeChannel.The mock setup for the detach retry test has been updated to use the new
RealtimeChannelinterface, maintaining the same test logic.chat-android/src/test/java/com/ably/chat/PresenceTest.kt (6)
3-8: Updated imports to reflect new SDK structure.The imports have been updated to:
- Add
DEFAULT_ROOM_IDandcreateMockRealtimeChannelfor test setup- Add
RealtimePresencefrom the newcom.ably.pubsubpackageThese changes align with the new SDK structure, enabling proper test setup with the new interfaces.
23-25: Updated variable declarations to use new types.The
pubSubPresenceis now declared as aRealtimePresencetype without direct initialization, which will be set up in thesetUpmethod.
29-33: Updated test setup to use new channel creation method.The test now:
- Creates a mock channel using the new
createMockRealtimeChannelmethod- Sets up the channels mock to return this channel
- Gets the presence from the channel
This approach aligns with the new SDK structure while maintaining the same test functionality.
43-43: Updated mock behavior for presence subscription.The mock now returns a relaxed mock instance instead of Unit, providing more flexibility in the test.
79-79: Updated mock behavior for presence subscription in another test.Consistently applying the same mock behavior change across all test methods.
116-116: Updated mock behavior for presence subscription in a third test.Consistently applying the same mock behavior change to all test methods that use presence subscription.
chat-android/src/test/java/com/ably/chat/TypingTest.kt (5)
5-5: Import for createMockRealtimeChannel
No issues with adding this import.
8-8: Import for RealtimePresence
No issues with adding this import.
27-27: Mocking RealtimePresence
Using a relaxed mock for presence is consistent with established patterns in these tests.
37-38: Mock channel creation and presence assignment
Channel is created and its presence is replaced with the new mock, ensuring RealtimePresence is tested thoroughly.
40-41: Mocking realtimeClient.channels
No issues found with the approach for returning the mock channel.chat-android/src/test/java/com/ably/chat/room/lifecycle/RetryTest.kt (34)
3-3: New import for InternalAPI
Using InternalAPI annotation is valid for marking experimental or non-public APIs usage.
49-49: Unmocking static attachCoroutine
This teardown step is consistent with the static mocking approach.
52-52: Opting into InternalAPI
This is appropriate for referencing annotated internal APIs.
57-58: Mock static attachCoroutine
The mocking approach looks correct and aligns with how MockK handles statics.
60-61: Captured detached channels
Capturing detached channels can help verify correct channel wind-down logic.
68-71: Initialization of channelWrapper and setting state
Assigning attached state is appropriate for test setup.
86-86: OptIn usage
Again, referencing InternalAPI for the test is consistent.
92-100: Mocking attach/detach with failure simulation
This block correctly simulates a failed channel when “typing” is in the name, ensuring suspended or failed states are tested.
128-131: Channel reattach with forced failure
Simulating channel failure for “typing” channels is consistent with test scenarios.
139-140: Assigning ChannelState.attached
Sets up the messages contributor channel in the attached state for test coverage.
150-159: OptIn annotation and channel attach/detach
Ensures all testable features (attach/detach) are mocked out for the presence test scenario.
163-170: Simulated once-listeners for attached and failed states
Correctly sets up callback behavior for ChannelState transitions.
183-184: Channel once attached
Demonstrates the test confirming presence of an attach callback.
187-187: No direct issues
Line referencing the channel or verifying logic is missing here in snippet, but no problem is evident.
190-190: No concerns
Single-line addition does not pose issues.
275-275: No concerns
Single-line addition.
277-278: Attaching static
Again, mocking static attachCoroutine for RealtimeChannel usage.
282-282: No concerns
Single-line addition.
299-300: Listening for ChannelState.failed
Ensures the test picks up channel failure promptly.
303-304: Checking if channel is already failed
Guards for immediate failure scenario.
388-388: No concerns
Single-line modification.
391-391: No concerns
Single-line addition.
399-400: Building failure message with channelWrapper.errorMessage
Provides more info on which feature failed.
406-406: No concerns
Single-line addition.
412-413: Channel state check
Handles unexpected channel states with an appropriate error.
476-477: Excluding or including channels
Ensures the specified contributor is not detached unless the room is failed.
481-484: Room failed or releasing check
Skips detaching an already-failed channel if the room is also in failed or releasing states.
486-486: No concerns
Single-line reference.
495-495: Detach call
Logic for detaching channel inside a try-catch block is valid.
499-502: Fail room if channel is failed
Correct approach to mark the entire room’s status as failed when channel fails to detach.
505-506: Building error info
Captures a custom message for debug/troubleshooting.
663-664: Ignore channel previously in failed state
Skipping channels already failed.
668-669: Ignore already-detached channels
Prevents redundant operations on already-detached state.
674-675: Detach call
Completes the release operation for channels.chat-android/src/main/java/com/ably/chat/Presence.kt (3)
5-6: New Realtime imports
Introducing RealtimeChannel and RealtimePresence aligns with the updated pub-sub approach.
151-152: channelWrapper and presence usage
Associating RealtimePresence with the channelWrapper is the intended design.
197-197: Subscription method
Refactored to directly subscribe to presence and convert to ChatSubscription—streamlines logic.chat-android/src/main/java/com/ably/chat/RoomLifecycleManager.kt (20)
3-3: Import for InternalAPI
Valid usage where internal API features are called.
242-242: Do attach with retry
Ensuring a separate function reference for the inline block—logic remains clear.
277-278: OptIn usage with javaChannel once
Enables usage of attachCoroutine or once-listener APIs.
282-282: State check for attached
Short-circuits the continuation if channel is already attached.
299-300: ChannelState.failed
Once-listener triggers an exception if the channel fails.
303-304: Immediate check for existing failed
Allows an early fail if the channel was already in a failed state.
388-388: No concerns
Single-line log statement.
391-391: No concerns
Single-line update.
399-400: Building error message
Uses channelWrapper’s errorMessage to clarify attach failures.
406-406: Channel state check
Switch to set room status to Failed or Suspended.
412-413: Unexpected channel state
Appropriately flags an error scenario.
476-477: Bypass wind-down for the except contributor
Aligns with the logic for doChannelWindDown.
481-484: Ignore failed channels if room is also in failing states
Avoids unnecessary re-detach attempts.
486-486: No concerns
Single-line reference in the code.
495-495: Detaching channels
Wraps the detach call in a try-catch for safe fallback.
499-502: Handling detach failure
If channel is failed and the room is still active, the room moves to failed.
505-506: Detachment error info
Custom error message for improved clarity.
663-664: Skip releasing already-failed channels
Ensures no repeated detach attempt for channels in a failed state.
668-669: Skip channels already in detached
Prevents repeated detach on an already-detached channel.
674-675: Detach operation
Completes the channel release logic without duplication.chat-android/src/main/java/com/ably/chat/Typing.kt (4)
5-6: New imports for SDK trackingThese imports enable the use of the new Kotlin-based interfaces for Ably's realtime channels and SDK tracking capabilities.
117-120: Good implementation of the SDK wrapper patternThe
channelWrapperproperty is now used as the primary interface, with the originalchannelproperty being derived from it. The@OptIn(InternalAPI::class)annotation correctly indicates that accessing the underlying Java channel is an internal implementation detail.
145-145: Updated presence subscription to use channelWrapperThe presence subscription is now obtained from the wrapper channel and converted to a chat subscription, maintaining consistent usage of the wrapper pattern.
160-160: Consistent usage of channelWrapper throughout the classAll channel operations have been updated to use the channelWrapper, providing consistent usage of the SDK tracking wrapper pattern.
Also applies to: 175-175, 185-185, 192-192
chat-android/src/main/java/com/ably/chat/RoomReactions.kt (4)
5-6: New imports for SDK trackingThese imports enable the use of the new Kotlin-based interfaces for Ably's realtime channels and SDK tracking capabilities.
117-120: Good implementation of the SDK wrapper patternThe
channelWrapperproperty is now used as the primary interface, with the originalchannelproperty being derived from it. The@OptIn(InternalAPI::class)annotation correctly indicates that accessing the underlying Java channel is an internal implementation detail.
146-146: Consistent usage of channelWrapper throughout the classAll channel operations (publish, subscribe, release) have been updated to use the channelWrapper, providing consistent usage of the SDK tracking wrapper pattern.
Also applies to: 167-167, 171-171
161-162: Appropriate handling of JSON metadataThe code correctly handles the metadata field as a JsonObject, which aligns with server behavior as noted in previous feedback.
chat-android/src/main/java/com/ably/chat/ChatClient.kt (4)
3-4: Added SDK tracking capabilitiesThese additions enable the tracking functionality mentioned in the PR objectives, allowing the SDK to inject agent information.
Also applies to: 42-42
31-31: Updated API type from RealtimeClient to AblyRealtimeThis change explicitly uses the concrete AblyRealtime type instead of a type alias, improving clarity.
Also applies to: 39-39, 45-45
49-53: Implemented SDK wrapper with agent trackingThe wrapper is created with the Chat SDK version information, enabling tracking of SDK usage as required. This is the core implementation of the feature requested in the PR objectives.
65-65: Consistent usage of realtimeClientWrapperAll instances where the realtime client was previously used now use the wrapper, ensuring proper tracking throughout the client's lifecycle. The connection and client ID references have also been updated appropriately.
Also applies to: 68-68, 76-76, 81-81, 90-91
chat-android/src/main/java/com/ably/chat/ChatApi.kt (4)
3-4: Improved type safety with HttpMethod enumUsing the HttpMethod enum instead of string literals improves type safety and makes the code more self-documenting.
35-35: Replaced string literals with enum values for HTTP methodsAll HTTP method string literals have been replaced with their corresponding enum values, which prevents typos and makes the code more robust.
Also applies to: 67-67, 97-97, 127-127, 153-153
163-163: Updated method signature and invocation for makeAuthorizedRequestThe method now accepts an HttpMethod enum instead of a string, and the parameters are properly ordered in the invocation.
Also applies to: 168-173
197-197: Updated method signature and invocation for makeAuthorizedPaginatedRequestThe method now accepts an HttpMethod enum instead of a string, and the parameters are properly ordered in the invocation.
Also applies to: 202-207
chat-android/src/main/java/com/ably/chat/Messages.kt (12)
3-3: Verify usage of internal annotation
The@InternalAPIannotation indicates that this API may be unstable or subject to change. Ensure there's no accidental reliance on internal contracts.
5-5: New RealtimeChannel import
The import ofRealtimeChannelfromcom.ably.pubsubappears intentional. No immediate issues.
319-320: Check concurrency for channel retrieval
OverridingchannelWrapperwithrealtimeChannels.get(...)looks correct, but consider thread-safety if channels may be accessed from multiple threads.
321-322: Relying on internaljavaChannel
AccessingjavaChannelrequires@OptIn(InternalAPI::class). Ensure there's no stable alternative to avoid potential breakage when the library evolves.
342-343: Registering channelStateListener
Again relying onjavaChannelfrom an internal API. Confirm that event handling remains aligned with future library updates.
381-381: Subscription approach
CallingchannelWrapper.subscribeforPubSubEventName.ChatMessageis consistent with the new channel wrapper. No concerns here.
383-383: Conditionally setting subscription point
Verifying thatchannelWrapper.state == ChannelState.attachedbefore setting serial is correct to ensure the channel is indeed attached.
392-392: Proper subscription cleanup
subscription.unsubscribe()ensures no lingering listeners. Looks good.
440-440: Channel serial retrieval
ReturningchannelWrapper.properties.channelSerialis straightforward. The null check via?:on the next line is correctly handled.
445-445: Attach serial retrieval
Similarly returningchannelWrapper.properties.attachSerial. The fallback with?: throwis consistent with the channel serial approach.
450-451: Unbinding from channel
RemovingchannelStateListenerfromjavaChannelis essential for cleanup. No issues noted.
453-453: Releasing the channel reference
realtimeChannels.release(channelWrapper.name)properly frees the channel from the underlying collection, preventing resource leakage.chat-android/src/test/java/com/ably/chat/room/RoomTestHelpers.kt (6)
3-3: Verify need for @internalapi
Ensure that using@InternalAPIin tests aligns with stable usage patterns and won't cause test fragility.
19-20: Switch to new pubsub imports
AdoptingRealtimeChannelandRealtimeClientis consistent with the broader refactor. No issues noted.
23-23: Building channels
ImportingbuildRealtimeChannelhelps create real or spied channel instances. Looks appropriate for mocking.
36-46: Mocking RealtimeClient
This function sets up a comprehensive mock forchannels. The approach is valid, though watch for interactions that might need verifying or partial instrumentation.
50-61: Creating a mock RealtimeChannel
Spying on a built channel and overriding states/presence is a common pattern in tests. Confirm these behaviors remain consistent with real usage.
63-65: createMockChannel extension
Wrapping the channel retrieval with a spy is a neat approach. No significant concerns.chat-android/src/test/java/com/ably/chat/room/lifecycle/ReleaseTest.kt (16)
13-13: Importing RealtimeChannel
The new RealtimeChannel import aligns with the rest of the refactor. No concerns here.
47-47: Unmocking attachCoroutine
Ensures test isolation by unmocking static references toattachCoroutine. Good cleanup.
125-129: Test for sequential detaches
This test starts the scenario withRoomStatus.Attached, then verifies sequential detach. The setup is clear and logically consistent.
131-136: Static mocking of detachCoroutine
MockingdetachCoroutineis valid. Verify that no concurrency issues arise when capturing multiple channels.
137-139: Contributor mocks
The call tocreateRoomFeatureMocks()clarifies the usage sequence. Looks consistent with real usage in production code.
140-156: Release test validations
The test checks final state and ensures each channel is accounted for in the sequence of detach calls. Thorough coverage.
164-168: Capturing channels for partial detach
Verifies that the test logic captures channel usage. The approach is sound.
173-176: Simulate failed channel
Setting one channel toChannelState.failedis a good test scenario. Confirms release flow continues for other channels.
204-213: Simulate repeated detachment failures
The test is verifying a fallback/time-based approach. The 200ms delay plus repeated tries is well-handled.
239-258: Randomizing channel state
Testing random states (attached,suspended,detached,failed) verifies robust handling. Be mindful that random states can make the test non-deterministic unless carefully observed.
262-263: Validating final channel name matching
Ensures the correct channels are processed in sequence. No concerns.
280-281: Test naming
Explicit mention of “prevent leakage” clarifies test intent. Good for future maintainers.
282-316: Detaching and releasing
The test orchestrates mock detaches, then confirms every contributor is properly released. Thorough coverage.
317-318: Ensuring no concurrency leaks
Waits forroomLifecycle.atomicCoroutineScope().finishedProcessing. This is crucial for stable test completion.
319-322: Verifying release calls
Ensures each contributor’srelease()method is invoked exactly once. No issues found.
338-341: Detaching in final scenario
MockingdetachCoroutineagain ensures channel state moves todetached. This pattern is consistent with prior tests.chat-android/src/test/java/com/ably/chat/room/lifecycle/AttachTest.kt (1)
3-3: Implementation smoothly transitions to RealtimeChannel.The refactoring from
io.ably.lib.realtime.Channeltocom.ably.pubsub.RealtimeChannelhas been implemented consistently throughout the test class. The imports, mocking, and variable types have all been updated appropriately.Also applies to: 20-20, 59-62, 175-177
chat-android/src/test/java/com/ably/chat/MessagesTest.kt (4)
38-42: Successfully migrated from direct channel access to channelWrapper.The test setup has been properly updated to use
createMockRealtimeChanneland configure the mock with appropriate expectations. This aligns with the overall changes in the codebase to use the channelWrapper pattern.
92-92: Updated channel subscriptions to use channelWrapper.The subscription logic has been correctly updated to use
messages.channelWrapperinstead of direct channel access, maintaining consistency with the new architecture.Also applies to: 100-100
168-169: Properly refactored channel properties access.The code now uses
ChannelPropertiesfor channel state and properties instead of direct property access. This approach is more structured and aligns with the new RealtimeChannel interface requirements.Also applies to: 174-174, 192-195, 207-208
215-232: Refactored subscription test with improved slot capture.The subscription test has been refactored to use a more structured approach for capturing and testing multiple message listeners. The use of a mutable list to track captured slots is a good pattern for testing multiple subscriptions.
chat-android/src/main/java/com/ably/chat/Utils.kt (5)
3-4: Updated imports for new Kotlin-based interfaces.The imports have been correctly updated to use the new
com.ably.pubsub.RealtimeChannelandcom.ably.pubsub.RealtimePresenceinterfaces, aligning with the PR objectives.
19-30: Successfully migrated channel extension functions.The channel extension functions (
attachCoroutine,detachCoroutine,publishCoroutine) have been properly updated to useRealtimeChannelinstead ofChannel, maintaining the same functionality while using the new Kotlin interface.Also applies to: 31-41, 43-56
58-64: Simplified getCoroutine implementation.The
getCoroutinefunction has been refactored to directly pass parameters to the underlyinggetmethod, which simplifies the code while maintaining the same functionality.
66-115: Successfully migrated presence extension functions.The presence extension functions (
enterClientCoroutine,updateClientCoroutine,leaveClientCoroutine) have been properly updated to useRealtimePresenceinstead ofPubSubPresence, ensuring consistency with the new architecture.
191-193: Added new asChatSubscription extension function.The addition of the
asChatSubscriptionextension function provides a clean way to convert fromcom.ably.Subscriptionto the application'sSubscriptiontype, which supports the SDK tracking capabilities introduced in this PR.
ttypic
force-pushed
the
ECO-5063/add-sdk-tracking
branch
from
e1dc694 to
15f4698
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
chat-android/src/test/java/com/ably/chat/room/lifecycle/AttachTest.kt (1)
400-400: Fix typo in test method name.The test method name has an extra hyphen in "CHA--RL1h6" which should be "CHA-RL1h6".
-fun `(CHA--RL1h6, CHA-RC2e) When room enters failed state, when CHA-RL1h5 fails to detach, op will be repeated till all channels are detached`() = runTest { +fun `(CHA-RL1h6, CHA-RC2e) When room enters failed state, when CHA-RL1h5 fails to detach, op will be repeated till all channels are detached`() = runTest {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
chat-android/src/test/java/com/ably/chat/ChatApiTest.kt(1 hunks)chat-android/src/test/java/com/ably/chat/MessagesTest.kt(7 hunks)chat-android/src/test/java/com/ably/chat/OccupancyTest.kt(2 hunks)chat-android/src/test/java/com/ably/chat/PresenceTest.kt(5 hunks)chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt(2 hunks)chat-android/src/test/java/com/ably/chat/TestUtils.kt(4 hunks)chat-android/src/test/java/com/ably/chat/TypingTest.kt(2 hunks)chat-android/src/test/java/com/ably/chat/room/RoomFeatureSharedChannelTest.kt(2 hunks)chat-android/src/test/java/com/ably/chat/room/RoomTestHelpers.kt(3 hunks)chat-android/src/test/java/com/ably/chat/room/lifecycle/AttachTest.kt(12 hunks)chat-android/src/test/java/com/ably/chat/room/lifecycle/DetachTest.kt(9 hunks)chat-android/src/test/java/com/ably/chat/room/lifecycle/ReleaseTest.kt(7 hunks)chat-android/src/test/java/com/ably/chat/room/lifecycle/RetryTest.kt(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt
- chat-android/src/test/java/com/ably/chat/TestUtils.kt
- chat-android/src/test/java/com/ably/chat/room/RoomFeatureSharedChannelTest.kt
- chat-android/src/test/java/com/ably/chat/ChatApiTest.kt
- chat-android/src/test/java/com/ably/chat/room/lifecycle/DetachTest.kt
- chat-android/src/test/java/com/ably/chat/TypingTest.kt
- chat-android/src/test/java/com/ably/chat/room/lifecycle/RetryTest.kt
- chat-android/src/test/java/com/ably/chat/room/lifecycle/ReleaseTest.kt
- chat-android/src/test/java/com/ably/chat/OccupancyTest.kt
🧰 Additional context used
🧠 Learnings (2)
chat-android/src/test/java/com/ably/chat/PresenceTest.kt (1)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/test/java/com/ably/chat/PresenceTest.kt:33-33
Timestamp: 2024-11-28T11:08:38.559Z
Learning: The `RoomEnsureAttachedTest.kt` file contains tests that verify room operations are only performed when the room is in the `ATTACHED` state, including scenarios where operations fail when the room is not attached, succeed when it is attached, and proper error handling for invalid room states.
chat-android/src/test/java/com/ably/chat/room/lifecycle/AttachTest.kt (1)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt:44-50
Timestamp: 2024-11-28T11:08:42.524Z
Learning: The test cases for verifying behavior when the room is not in the ATTACHED state are covered in `chat-android/src/test/java/com/ably/chat/room/RoomEnsureAttachedTest.kt`.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: check
- GitHub Check: coverage
🔇 Additional comments (29)
chat-android/src/test/java/com/ably/chat/room/lifecycle/AttachTest.kt (10)
3-3: Verify imports of new SDK components.The PR introduces imports for
InternalAPIannotation andRealtimeChannelfrom the PubSub adapter, which aligns with the PR objective of adding SDK tracking for REST and Realtime calls.Also applies to: 20-20
60-60: LGTM - Consistent update of static mocking.The code correctly updates the unmock reference from the old Java interface to the new Kotlin-based
RealtimeChannelinterface.
174-177: LGTM - Compatible mock implementation for RealtimeChannel.The mock implementation is correctly updated to use the new
RealtimeChanneltype instead of the previousio.ably.lib.realtime.Channeltype.
190-190: Appropriate usage of channelWrapper.Updating the reference from
channel.nametochannelWrapper.namealigns with the new wrapped channel implementation that enables SDK tracking.
218-218: Consistent usage of channelWrapper throughout test code.The update correctly uses the new channel wrapper structure in the initialization of pending discontinuity events.
247-247: Usage of InternalAPI requires explicit opt-in.The
@OptIn(InternalAPI::class)annotation correctly acknowledges that you're using APIs marked as internal, which is appropriate for test code.
254-259: LGTM - Updated channel state mocking approach.The implementation correctly updates to use property accessors (
channel.stateandchannel.reason) instead of method calls for setting channel state, which is more idiomatic in Kotlin.
289-291: Consistent channel state handling throughout tests.The test consistently uses the property accessor pattern for setting channel state and reason, which is a good improvement in the Kotlin context.
367-371: LGTM - Updated channel type for detached channels collection.The code correctly updates the collection type to match the new
RealtimeChannelinterface.
409-411: LGTM - Consistent channel state manipulation pattern.The test consistently applies the property accessor pattern for channel state management.
chat-android/src/test/java/com/ably/chat/PresenceTest.kt (5)
3-7: Updated imports to support new Realtime interfaceThe imports have been updated to use the new
createMockRealtimeChannelhelper andRealtimePresenceinterface from the pubsub package, which aligns with the PR objective of adding SDK tracking for Rest and Realtime calls.
23-24: Good type safety improvement with RealtimePresenceChanged from directly mocking
PubSubPresenceto declaring a properly typedRealtimePresencevariable, which provides better type safety and reflects the actual implementation in production code.
29-32: Improved mock setup using standard channel patternsThe setup now uses the standardized
createMockRealtimeChannelhelper with proper channel naming pattern, which better reflects how channels are created and used in the real implementation. This change will make tests more maintainable as they now mirror production code more closely.
43-43: Consistent mock configuration for subscriptionsUpdated to return a relaxed mock for the subscription, which provides more flexibility during testing while maintaining the same behavior.
79-79: Consistent usage of relaxUnitFun parameter across test casesThe update standardizes on using
relaxUnitFun = truefor mocking the return value of thesubscribemethod, which is appropriate for methods that return Unit.Also applies to: 116-116
chat-android/src/test/java/com/ably/chat/room/RoomTestHelpers.kt (6)
3-4: Added InternalAPI annotation supportThe addition of
InternalAPIimport is necessary to support the use of internal APIs in the test helpers.
19-21: Updated imports for new Realtime interfacesThe code now imports
RealtimeChannelandRealtimeClientfrom the pubsub package, aligning with the PR's objective to use the new Kotlin-based interfaces.
36-48: Refactored client mock to use new RealtimeClient interfaceThe
createMockRealtimeClientfunction has been updated to return a mock ofRealtimeClientwith proper setup for channel access. This change properly reflects how the Realtime client is used in production code and supports the new SDK tracking capability.
50-61: Added helper for creating RealtimeChannel mocksThe new
createMockRealtimeChannelfunction provides a standardized way to create mock channels that closely mirror the behavior of real channels. This is a well-structured addition that will improve test consistency.Note that the function correctly sets up:
- Channel name
- Java channel integration
- Initial state
- Presence functionality
- Subscription methods
63-65: Redesigned createMockChannel as an extension functionThe
createMockChannelfunction has been updated to be an extension function onRealtimeClient, which properly reflects the relationship between clients and channels. This change makes the test code more closely match the production code architecture.
121-125: Improved contributor mocking approachThe test helper now uses
spykon the actual room properties instead of creating new instances of the default implementations. This approach is more maintainable as it will automatically adapt to any changes in the underlying implementations.chat-android/src/test/java/com/ably/chat/MessagesTest.kt (8)
3-4: Added support for InternalAPI annotationThe import of
InternalAPIannotation is necessary to access internal APIs in the test code.
5-6: Updated to use new mock channel helperReplaced
createMockChannelimport withcreateMockRealtimeChannelto align with the new channel architecture.
13-14: Added ChannelProperties import for property accessThe code now imports
ChannelPropertieswhich allows for more structured access to channel metadata.
35-42: Updated setUp method to use new channel architectureThe setup has been refactored to create a
RealtimeChannelmock usingcreateMockRealtimeChanneland properly configure the client to return it. This approach better reflects how channels are created and used in production code.
92-92: Updated subscription test to use channelWrapperThe test now uses
messages.channelWrapperinstead of directly accessing the channel, which encapsulates the channel interaction logic and aligns with how subscriptions work in the production code.Also applies to: 100-100
168-169: Improved channel state and properties mockingThe test now properly mocks the channel state and properties using the
ChannelPropertiesclass, which provides a more structured way to set and verify channel metadata like serials.Also applies to: 174-174
186-195: Enhanced channel serial testing with proper propertiesThe test for channel serial updates now uses
ChannelPropertiesto properly set both the channel serial and attach serial, which better reflects how these properties are managed in the actual implementation.
219-232: Refactored message subscription test for better slot managementThe test now captures the subscription listener slots in a list, which allows for more flexible testing of multiple subscriptions. This approach enables the test to verify that each subscriber receives events independently.
To track Chat SDK usage, we replaced the old ably-java interfaces for Channel, Client, and Presence with new Kotlin-based interfaces. The new Kotlin interfaces include the `createWrapperSDKProxy` method, which injects agent information for the new set of agents.
changed some test to support new kotlin RealtimeChannel and RealtimeClient objects
ttypic
force-pushed
the
ECO-5063/add-sdk-tracking
branch
from
15f4698 to
e419db6
Compare
ttypic
force-pushed
the
ECO-5063/add-sdk-tracking
branch
from
e419db6 to
abe46a2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
chat-android/src/test/java/com/ably/chat/integration/EmbeddedServer.kt (1)
9-55: 🛠️ Refactor suggestionFix indentation throughout the file
The pipeline reports numerous indentation warnings throughout the file. The code uses 2-space indentation while the project standard is 4-space indentation.
Adjust the indentation of all lines to use 4 spaces instead of 2 spaces to match the project standard.
🧰 Tools
🪛 GitHub Actions: Check
[warning] 10-10: Unexpected indentation (2) (should be 4) [Indentation]
[warning] 11-11: Unexpected indentation (2) (should be 4) [Indentation]
[warning] 12-12: Unexpected indentation (2) (should be 4) [Indentation]
[warning] 16-16: Unexpected indentation (2) (should be 4) [Indentation]
[warning] 17-17: Unexpected indentation (2) (should be 4) [Indentation]
[warning] 21-21: Unexpected indentation (2) (should be 4) [Indentation]
[warning] 22-22: Unexpected indentation (2) (should be 4) [Indentation]
[warning] 26-26: Unexpected indentation (2) (should be 4) [Indentation]
[warning] 30-30: Unexpected indentation (2) (should be 4) [Indentation]
[warning] 31-31: Unexpected indentation (4) (should be 8) [Indentation]
[warning] 32-32: Unexpected indentation (4) (should be 8) [Indentation]
[warning] 35-35: Unexpected indentation (2) (should be 4) [Indentation]
[warning] 37-37: Unexpected indentation (2) (should be 4) [Indentation]
[warning] 39-39: Unexpected indentation (4) (should be 8) [Indentation]
[warning] 40-40: Unexpected indentation (6) (should be 12) [Indentation]
[warning] 41-41: Unexpected indentation (6) (should be 12) [Indentation]
[warning] 43-43: Unexpected indentation (6) (should be 12) [Indentation]
[warning] 44-44: Unexpected indentation (4) (should be 8) [Indentation]
[warning] 45-45: Unexpected indentation (4) (should be 8) [Indentation]
[warning] 49-49: Unexpected indentation (2) (should be 4) [Indentation]
[warning] 52-52: Unexpected indentation (2) (should be 4) [Indentation]
🧹 Nitpick comments (8)
chat-android/src/main/java/com/ably/chat/Occupancy.kt (1)
114-114: Improved subscription handlingUsing
channelWrapper.subscribe().asChatSubscription()simplifies the subscription management and creates a consistent pattern across the codebase.chat-android/src/main/java/com/ably/chat/Presence.kt (1)
197-197: Simplified subscription managementThe subscription handling has been improved to directly return the converted subscription object using
asChatSubscription(), removing unnecessary manual subscription handling.This change simplifies the code by leveraging the new wrapper's subscription handling capabilities.
chat-android/src/test/java/com/ably/chat/integration/RequestHeaderTest.kt (3)
27-29: Add trailing comma as required by the linterThe pipeline is reporting a warning about a missing trailing comma before the closing parenthesis.
Assert.assertTrue( - agents.contains("chat-kotlin/${com.ably.chat.BuildConfig.APP_VERSION}") + agents.contains("chat-kotlin/${com.ably.chat.BuildConfig.APP_VERSION}"), )🧰 Tools
🪛 GitHub Actions: Check
[warning] 28-28: Missing trailing comma before ")" [TrailingCommaOnCallSite]
35-36: Use underscores in numeric literals for readabilityThe pipeline is reporting a warning that the number should be separated by underscores for better readability.
- const val PORT = 27332 + const val PORT = 27_332 lateinit var server: EmbeddedServer🧰 Tools
🪛 GitHub Actions: Check
[warning] 35-35: This number should be separated by underscores in order to increase readability. [UnderscoresInNumericLiterals]
59-70: Use a constant or configure credentials securelyThe
createAblyRealtime()method is using a hardcoded API key. Even though this is a test, it's best practice to avoid placing credentials in the source code.Consider using a test constant, environment variable, or a test-specific configuration file for the API key value.
chat-android/src/test/java/com/ably/chat/room/lifecycle/AttachTest.kt (1)
399-400: Fix typo in test nameThere's an extra hyphen in the test name that should be fixed for consistency.
- fun `(CHA--RL1h6, CHA-RC2e) When room enters failed state, when CHA-RL1h5 fails to detach, op will be repeated till all channels are detached`() = runTest { + fun `(CHA-RL1h6, CHA-RC2e) When room enters failed state, when CHA-RL1h5 fails to detach, op will be repeated till all channels are detached`() = runTest {chat-android/src/test/java/com/ably/chat/integration/EmbeddedServer.kt (2)
1-8: Fix import orderingAccording to the pipeline warning, the imports should be ordered according to the pattern specified in .editorconfig.
Rearrange the imports to match the project's import ordering pattern.
🧰 Tools
🪛 GitHub Actions: Check
[warning] 3-3: Imports must be ordered according to the pattern specified in .editorconfig [ImportOrdering]
[warning] 1-1: Unexpected indentation (2) (should be 4) [Indentation]
15-18: Implement equals() and hashCode() for Response classThe
Responseclass contains aByteArrayfield, which doesn't have properequals()andhashCode()implementations by default. Since this is a data class, you should override these methods to ensure proper equality checking.data class Response( val mimeType: String, val data: ByteArray, ) { + override fun equals(other: Any?): Boolean { + if (this === other) return true + if (javaClass != other?.javaClass) return false + + other as Response + + if (mimeType != other.mimeType) return false + if (!data.contentEquals(other.data)) return false + + return true + } + + override fun hashCode(): Int { + var result = mimeType.hashCode() + result = 31 * result + data.contentHashCode() + return result + } }🧰 Tools
🪛 GitHub Actions: Check
[warning] 16-16: Unexpected indentation (2) (should be 4) [Indentation]
[warning] 17-17: Unexpected indentation (2) (should be 4) [Indentation]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
chat-android/build.gradle.kts(1 hunks)chat-android/src/main/java/com/ably/chat/ChatApi.kt(8 hunks)chat-android/src/main/java/com/ably/chat/ChatClient.kt(4 hunks)chat-android/src/main/java/com/ably/chat/Discontinuities.kt(1 hunks)chat-android/src/main/java/com/ably/chat/Messages.kt(6 hunks)chat-android/src/main/java/com/ably/chat/Occupancy.kt(3 hunks)chat-android/src/main/java/com/ably/chat/Presence.kt(3 hunks)chat-android/src/main/java/com/ably/chat/Room.kt(3 hunks)chat-android/src/main/java/com/ably/chat/RoomLifecycleManager.kt(7 hunks)chat-android/src/main/java/com/ably/chat/RoomReactions.kt(4 hunks)chat-android/src/main/java/com/ably/chat/Rooms.kt(1 hunks)chat-android/src/main/java/com/ably/chat/Typing.kt(6 hunks)chat-android/src/main/java/com/ably/chat/Utils.kt(8 hunks)chat-android/src/test/java/com/ably/chat/ChatApiTest.kt(1 hunks)chat-android/src/test/java/com/ably/chat/MessagesTest.kt(7 hunks)chat-android/src/test/java/com/ably/chat/OccupancyTest.kt(2 hunks)chat-android/src/test/java/com/ably/chat/PresenceTest.kt(5 hunks)chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt(2 hunks)chat-android/src/test/java/com/ably/chat/TestUtils.kt(4 hunks)chat-android/src/test/java/com/ably/chat/TypingTest.kt(2 hunks)chat-android/src/test/java/com/ably/chat/integration/EmbeddedServer.kt(1 hunks)chat-android/src/test/java/com/ably/chat/integration/MessagesIntegrationTest.kt(2 hunks)chat-android/src/test/java/com/ably/chat/integration/RequestHeaderTest.kt(1 hunks)chat-android/src/test/java/com/ably/chat/room/RoomFeatureSharedChannelTest.kt(2 hunks)chat-android/src/test/java/com/ably/chat/room/RoomTestHelpers.kt(3 hunks)chat-android/src/test/java/com/ably/chat/room/lifecycle/AttachTest.kt(12 hunks)chat-android/src/test/java/com/ably/chat/room/lifecycle/DetachTest.kt(9 hunks)chat-android/src/test/java/com/ably/chat/room/lifecycle/ReleaseTest.kt(7 hunks)chat-android/src/test/java/com/ably/chat/room/lifecycle/RetryTest.kt(8 hunks)chat-android/src/test/java/io/ably/lib/realtime/RealtimeUtils.kt(2 hunks)example/src/main/java/com/ably/chat/example/MainActivity.kt(2 hunks)gradle/libs.versions.toml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (19)
- chat-android/src/test/java/com/ably/chat/ChatApiTest.kt
- chat-android/src/main/java/com/ably/chat/Rooms.kt
- chat-android/src/test/java/com/ably/chat/TestUtils.kt
- chat-android/src/test/java/com/ably/chat/OccupancyTest.kt
- example/src/main/java/com/ably/chat/example/MainActivity.kt
- chat-android/src/test/java/com/ably/chat/room/RoomFeatureSharedChannelTest.kt
- chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt
- chat-android/src/main/java/com/ably/chat/Discontinuities.kt
- chat-android/src/test/java/com/ably/chat/PresenceTest.kt
- chat-android/src/test/java/com/ably/chat/room/lifecycle/DetachTest.kt
- chat-android/src/test/java/com/ably/chat/room/RoomTestHelpers.kt
- chat-android/src/main/java/com/ably/chat/Messages.kt
- chat-android/src/test/java/com/ably/chat/TypingTest.kt
- gradle/libs.versions.toml
- chat-android/src/test/java/com/ably/chat/room/lifecycle/ReleaseTest.kt
- chat-android/src/main/java/com/ably/chat/Utils.kt
- chat-android/src/main/java/com/ably/chat/ChatApi.kt
- chat-android/src/main/java/com/ably/chat/ChatClient.kt
- chat-android/src/main/java/com/ably/chat/Room.kt
🧰 Additional context used
🧠 Learnings (3)
chat-android/src/main/java/com/ably/chat/RoomReactions.kt (1)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#100
File: chat-android/src/main/java/com/ably/chat/RoomReactions.kt:156-156
Timestamp: 2025-02-14T13:27:05.487Z
Learning: The server always ensures that the metadata field in room reactions is sent as a JsonObject, eliminating the need for explicit type checking on the client side.
chat-android/src/test/java/com/ably/chat/room/lifecycle/AttachTest.kt (1)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt:44-50
Timestamp: 2024-11-28T11:08:42.524Z
Learning: The test cases for verifying behavior when the room is not in the ATTACHED state are covered in `chat-android/src/test/java/com/ably/chat/room/RoomEnsureAttachedTest.kt`.
chat-android/src/test/java/com/ably/chat/room/lifecycle/RetryTest.kt (1)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt:44-50
Timestamp: 2024-11-28T11:08:42.524Z
Learning: The test cases for verifying behavior when the room is not in the ATTACHED state are covered in `chat-android/src/test/java/com/ably/chat/room/RoomEnsureAttachedTest.kt`.
🪛 GitHub Actions: Check
chat-android/src/test/java/com/ably/chat/integration/MessagesIntegrationTest.kt
[warning] 260-260: Missing trailing comma before ")" [TrailingCommaOnCallSite]
chat-android/src/test/java/com/ably/chat/integration/RequestHeaderTest.kt
[warning] 28-28: Missing trailing comma before ")" [TrailingCommaOnCallSite]
[warning] 35-35: This number should be separated by underscores in order to increase readability. [UnderscoresInNumericLiterals]
chat-android/src/test/java/com/ably/chat/integration/EmbeddedServer.kt
[warning] 3-3: Imports must be ordered according to the pattern specified in .editorconfig [ImportOrdering]
[warning] 10-10: Unexpected indentation (2) (should be 4) [Indentation]
[warning] 11-11: Unexpected indentation (2) (should be 4) [Indentation]
[warning] 12-12: Unexpected indentation (2) (should be 4) [Indentation]
[warning] 16-16: Unexpected indentation (2) (should be 4) [Indentation]
[warning] 17-17: Unexpected indentation (2) (should be 4) [Indentation]
[warning] 21-21: Unexpected indentation (2) (should be 4) [Indentation]
[warning] 22-22: Unexpected indentation (2) (should be 4) [Indentation]
[warning] 26-26: Unexpected indentation (2) (should be 4) [Indentation]
[warning] 30-30: Unexpected indentation (2) (should be 4) [Indentation]
[warning] 31-31: Unexpected indentation (4) (should be 8) [Indentation]
[warning] 32-32: Unexpected indentation (4) (should be 8) [Indentation]
[warning] 35-35: Unexpected indentation (2) (should be 4) [Indentation]
[warning] 37-37: Unexpected indentation (2) (should be 4) [Indentation]
[warning] 39-39: Unexpected indentation (4) (should be 8) [Indentation]
[warning] 40-40: Unexpected indentation (6) (should be 12) [Indentation]
[warning] 41-41: Unexpected indentation (6) (should be 12) [Indentation]
[warning] 43-43: Unexpected indentation (6) (should be 12) [Indentation]
[warning] 44-44: Unexpected indentation (4) (should be 8) [Indentation]
[warning] 45-45: Unexpected indentation (4) (should be 8) [Indentation]
[warning] 49-49: Unexpected indentation (2) (should be 4) [Indentation]
[warning] 52-52: Unexpected indentation (2) (should be 4) [Indentation]
[warning] 1-1: Unexpected indentation (2) (should be 4) [Indentation]
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: coverage
🔇 Additional comments (48)
chat-android/src/main/java/com/ably/chat/RoomLifecycleManager.kt (6)
3-3: Added import for InternalAPI annotation.This import is necessary for using the
@OptIn(InternalAPI::class)annotations that are now required when accessing the Java channel through the wrapper.
243-243: Updated channel access pattern for listener registration and state checks.The code has been refactored to use
channelWrapperinstead of direct channel access, with proper usage of@OptIn(InternalAPI::class)annotations when accessing the underlying Java implementation throughjavaChannel. This change supports the SDK tracking functionality mentioned in the PR objectives.Also applies to: 277-278, 282-282, 298-299, 303-304
391-391: Updated channel operations in doAttach method.The
doAttachmethod now correctly useschannelWrapperfor attaching channels and accessing state and error information. The error messages have been updated to include channelWrapper's error information.Also applies to: 399-399, 406-406, 412-412
476-476: Updated channel comparison and state checks in doChannelWindDown.The
doChannelWindDownmethod now correctly compareschannelWrapperinstances and checks states through the wrapper. This maintains consistent channel access patterns throughout the codebase.Also applies to: 482-486
494-494: Updated channel detachment and error handling in doChannelWindDown.Channel detachment now uses
channelWrapper.detachCoroutine()and error handling has been updated to check states and construct error messages using the wrapper. This maintains a consistent approach across the codebase.Also applies to: 499-505
663-674: Updated channel state checks and operations in doRelease.The
doReleasemethod now properly useschannelWrapperfor state checks and the detachment operation, completing the consistent transition to using the wrapper throughout the lifecycle management code.chat-android/src/test/java/io/ably/lib/realtime/RealtimeUtils.kt (1)
42-43: Well-designed extension property for test utilitiesThis extension property provides a clean way to access channel options, which will be useful for testing agent parameters as seen in other changes.
chat-android/src/test/java/com/ably/chat/integration/MessagesIntegrationTest.kt (1)
253-262:✅ Verification successful
Good test for SDK tracking integration
This new test verifies that the agent information is properly included in the channel parameters, which aligns well with the PR objective of adding SDK tracking for Rest and Realtime calls.
🏁 Script executed:
#!/bin/bash # Check other test files to see if similar agent tests are needed rg -A 2 "channel\.channelOptions\?.params\?.get\(\"agent\"\)" --type kotlinLength of output: 402
Approved: Agent Parameter Testing Verified
The test in
chat-android/src/test/java/com/ably/chat/integration/MessagesIntegrationTest.ktcorrectly verifies that the channel parameters include the expected agent information, ensuring the SDK tracking integration works as intended. The repository search confirms that this is the only instance where this agent parameter is validated, so no additional similar tests are required.🧰 Tools
🪛 GitHub Actions: Check
[warning] 260-260: Missing trailing comma before ")" [TrailingCommaOnCallSite]
chat-android/src/main/java/com/ably/chat/Occupancy.kt (1)
5-5: Good implementation of SDK wrapper for trackingAdding the RealtimeChannel wrapper aligns with the PR objective of replacing old Ably Java interfaces with new Kotlin interfaces for tracking purposes.
Also applies to: 89-90
chat-android/build.gradle.kts (2)
54-54: Essential dependency added for SDK trackingThe addition of the Ably PubSub adapter dependency is essential for implementing the SDK tracking functionality described in the PR objectives.
62-63:✅ Verification successful
Appropriate test dependencies
Adding nanohttpd and turbine libraries will help with testing the new functionality, particularly for HTTP interactions and async flow testing.
🏁 Script executed:
#!/bin/bash # Check if the new dependencies are used in test files echo "Checking for nanohttpd usage:" rg -A 2 "nanohttpd|NanoHTTPD" --type kotlin echo "Checking for turbine usage:" rg -A 2 "turbine|TestTurbine" --type kotlinLength of output: 2880
Test Dependencies Verified and Approved
The nanohttpd and turbine dependencies have been confirmed in the test code. NanoHTTPD usage is evident in files such as
chat-android/src/test/java/com/ably/chat/integration/RequestHeaderTest.ktandchat-android/src/test/java/com/ably/chat/integration/EmbeddedServer.kt, and the turbine library is used inRequestHeaderTest.kt. These dependencies appropriately support HTTP interaction tests and asynchronous flow testing.chat-android/src/test/java/com/ably/chat/room/lifecycle/RetryTest.kt (11)
3-3: New imports added for RealtimeChannel refactoringThese imports support the transition from using direct Ably channel references to using the new RealtimeChannel wrapper, which aligns with the SDK tracking enhancement mentioned in the PR objectives.
Also applies to: 16-16
49-49: Updated static mock teardownThe static unmocking now correctly references
RealtimeChannel::attachCoroutineinstead of the previousChannel::attachCoroutine, maintaining consistency with the new wrapper implementation.
52-59: Updated mock setup for RealtimeChannelThe test now properly uses
@OptIn(InternalAPI::class)and mocks the static methods onRealtimeChannelrather thanChannel. This ensures the test remains functional with the new channel wrapper implementation.
60-62: Updated type in captured detached channelsThe list now correctly captures
RealtimeChannelobjects instead ofChannelobjects, maintaining type consistency with the refactored implementation.
68-70: Updated channel access patternThe test now correctly accesses the channel state through the
channelWrapperproperty and sets the state on the underlying Java channel throughjavaChannel.setState(). This follows the new pattern established in the PR.
86-100: Properly updated mocks and channel state handlingThe test correctly uses
@OptIn(InternalAPI::class)and updates all references to the channel to use the newchannelWrapperproperty. Channel state manipulation now properly usesjavaChannel.setState().
107-109: Updated channel access in function setupThe test now properly uses
channelWrapperto access the channel and set its state, maintaining consistency with the rest of the codebase changes.
125-134: Updated mock setup for RealtimeChannel in additional testThe test now correctly mocks the static methods on
RealtimeChanneland captures the correct type. The pattern is consistent with other test cases in the file.
139-140: Updated channel access in third testConsistent with the pattern established in previous tests, this test now accesses the channel through the
channelWrapperproperty.
150-187: Updated channel references in tests with listenersAll references to the channel have been properly updated to use
javaChannelwhen interacting with channel state listeners. The test correctly verifies that the right methods are called on the Java channel.
190-207: Updated channel access pattern in final testThe test follows the established pattern of using
@OptIn(InternalAPI::class)and accessing the state throughchannelWrapper, while manipulating the state throughjavaChannel.chat-android/src/main/java/com/ably/chat/Presence.kt (3)
5-6: Added imports for RealtimeChannel and RealtimePresenceThese imports support the new channel wrapper implementation which is part of the SDK tracking enhancement.
151-152: Added channelWrapper propertyProperly added the
channelWrapperproperty to expose the RealtimeChannel from the room's messages feature, following the pattern established across the codebase.
155-155: Updated presence field to use RealtimePresenceThe presence property now correctly uses the wrapper's presence property, which provides stronger typing and better SDK tracking capabilities.
chat-android/src/main/java/com/ably/chat/Typing.kt (4)
5-6: Added imports for InternalAPI and RealtimeChannelThese imports support the transition to using the new RealtimeChannel wrapper for improved SDK tracking capabilities.
117-120: Added channelWrapper property and updated channel accessThe implementation now correctly:
- Creates a
channelWrapperproperty using the RealtimeClient's channels accessor- Exposes the original channel interface through
javaChannelwith appropriate internal API annotationThis maintains backward compatibility while enabling the new SDK tracking features.
145-145: Updated presence subscriptionThe presence subscription now correctly uses the wrapper's presence object and simplifies subscription handling with
asChatSubscription().
160-160: Updated all presence method callsAll presence method calls (get, enter, leave, and release) now correctly use the channelWrapper's presence object, maintaining a consistent pattern throughout the implementation.
Also applies to: 175-175, 185-185, 192-192
chat-android/src/main/java/com/ably/chat/RoomReactions.kt (5)
5-6: Added imports for InternalAPI and RealtimeChannelThese imports support the transition to using the new RealtimeChannel wrapper for improved SDK tracking.
117-120: Added channelWrapper property with appropriate internal API handlingThe implementation now correctly:
- Creates a
channelWrapperproperty using the RealtimeClient's channels accessor- Exposes the original channel interface through
javaChannelwith appropriate internal API annotationThis approach maintains backward compatibility while enabling new SDK tracking features.
146-146: Updated publish method to use channelWrapperThe
publishCoroutinemethod is now correctly called on the channelWrapper instead of directly on the channel, following the established pattern.
167-167: Improved subscription handlingThe subscription management has been simplified to use
asChatSubscription()directly, creating a more maintainable and consistent approach to subscription handling across the codebase.
171-171: Updated channel release to use channelWrapperThe channel release now correctly references the channelWrapper's name, maintaining consistency with the rest of the implementation.
chat-android/src/test/java/com/ably/chat/integration/RequestHeaderTest.kt (1)
16-31: Test structure looks goodThe test correctly verifies that ChatClient adds an "ably-agent" header with the appropriate version information in wrapper SDK client calls, which aligns with the PR objective of enhancing SDK tracking.
🧰 Tools
🪛 GitHub Actions: Check
[warning] 28-28: Missing trailing comma before ")" [TrailingCommaOnCallSite]
chat-android/src/test/java/com/ably/chat/MessagesTest.kt (6)
3-7: Clean import updateThe imports have been correctly updated to match the new structure using Kotlin-based interfaces, which aligns with the PR's objective of replacing old Ably Java interfaces.
35-46: Well-implemented update to use RealtimeChannelThe setUp method has been properly refactored to use the new
createMockRealtimeChannelmethod and correctly sets up the test environment with the new channel wrapper structure.
89-101: Consistent implementation of channel wrapperThe subscription test has been correctly updated to use
channelWrapperinstead of directly accessing the channel, which properly encapsulates the channel operations and improves the code structure.
167-170: Clean transition to ChannelPropertiesThe test has been properly updated to use
ChannelPropertiesinstead of direct property manipulation, which provides better encapsulation and aligns with the new interface structure.
185-209: Comprehensive update to channel property accessThe test for channel serial updates after reattach has been thoroughly updated to use the new properties structure, maintaining the original test coverage while adapting to the new interface.
219-234: Proper slot capture implementationThe test for message subscription invocation has been properly updated to use the channel wrapper, with correct handling of the captured pubSubMessageListener slot.
chat-android/src/test/java/com/ably/chat/room/lifecycle/AttachTest.kt (6)
3-21: Appropriate import updatesThe imports have been correctly updated to include
InternalAPIannotation and useRealtimeChannelinstead ofChannel, which aligns with the PR's objective of using new Kotlin-based interfaces.
58-61: Correct static mocking updateThe tearDown method has been properly updated to unmock
RealtimeChannel::attachCoroutineinstead of the previous implementation, ensuring proper test cleanup.
174-178: Clean transition to RealtimeChannelThe mocking of the
attachCoroutinemethod has been properly updated to useRealtimeChannelinstead ofChannel, with the captured channel type appropriately changed as well.
188-196: Consistent use of channelWrapper.nameReferences to channel names have been properly updated to use
channelWrapper.name, maintaining the test's original functionality while adapting to the new interface structure.
247-250: Good use of OptIn annotationThe
@OptIn(InternalAPI::class)annotation has been properly added to indicate the use of internal APIs, which helps with code clarity and maintainability.
253-262: Channel state properties properly usedThe code for simulating channel states has been updated to use property accessors (
stateandreason) instead of previous method calls, which aligns with the new interface structure.chat-android/src/test/java/com/ably/chat/integration/EmbeddedServer.kt (1)
29-47: Well-structured embedded server implementationThe
EmbeddedServerclass is well-implemented, usingMutableSharedFlowto track served requests and providing a clean API for testing HTTP interactions. This implementation will be valuable for testing the SDK tracking features in the PR.🧰 Tools
🪛 GitHub Actions: Check
[warning] 30-30: Unexpected indentation (2) (should be 4) [Indentation]
[warning] 31-31: Unexpected indentation (4) (should be 8) [Indentation]
[warning] 32-32: Unexpected indentation (4) (should be 8) [Indentation]
[warning] 35-35: Unexpected indentation (2) (should be 4) [Indentation]
[warning] 37-37: Unexpected indentation (2) (should be 4) [Indentation]
[warning] 39-39: Unexpected indentation (4) (should be 8) [Indentation]
[warning] 40-40: Unexpected indentation (6) (should be 12) [Indentation]
[warning] 41-41: Unexpected indentation (6) (should be 12) [Indentation]
[warning] 43-43: Unexpected indentation (6) (should be 12) [Indentation]
[warning] 44-44: Unexpected indentation (4) (should be 8) [Indentation]
[warning] 45-45: Unexpected indentation (4) (should be 8) [Indentation]
chat-android/src/test/java/com/ably/chat/integration/MessagesIntegrationTest.kt
Outdated
Show resolved
Hide resolved
Added tests that checks channel params and rest request headers contains agent information
ttypic
force-pushed
the
ECO-5063/add-sdk-tracking
branch
from
abe46a2 to
8c57f63
Compare
chat-android/src/test/java/com/ably/chat/integration/MessagesIntegrationTest.kt
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
chat-android/src/test/java/com/ably/chat/integration/MessagesIntegrationTest.kt (1)
253-262: Test added for agent tracking in channel parametersThe new test verifies that the messages channel includes the correct agent parameter, which aligns with the PR objective of enhancing SDK tracking.
Consider improving the assertion message to provide better context in case of failure:
assertEquals( "chat-kotlin/${BuildConfig.APP_VERSION}", room.messages.channel.channelOptions?.params?.get("agent"), + "Channel should include the correct agent parameter" )chat-android/src/test/java/com/ably/chat/integration/RequestHeaderTest.kt (2)
33-57: Companion object for server setup and teardownThe test properly manages the embedded server lifecycle with appropriate setup and teardown methods.
Consider using a dynamic port allocation instead of a hardcoded port number to prevent potential port conflicts during parallel test execution:
- const val PORT = 27_332 + val PORT = findAvailablePort() + + private fun findAvailablePort(): Int { + return ServerSocket(0).use { it.localPort } + }
59-70: Client creation helper functionThe function sets up the Ably client with appropriate test configurations.
Replace the hardcoded credentials with a constant or test-specific value:
- val options = ClientOptions("xxxxx:yyyyyyy").apply { + val options = ClientOptions(TEST_API_KEY).apply {Where
TEST_API_KEYcould be defined as a constant in the companion object.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
chat-android/build.gradle.kts(1 hunks)chat-android/src/test/java/com/ably/chat/integration/EmbeddedServer.kt(1 hunks)chat-android/src/test/java/com/ably/chat/integration/MessagesIntegrationTest.kt(2 hunks)chat-android/src/test/java/com/ably/chat/integration/RequestHeaderTest.kt(1 hunks)chat-android/src/test/java/io/ably/lib/realtime/RealtimeUtils.kt(2 hunks)gradle/libs.versions.toml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- chat-android/src/test/java/io/ably/lib/realtime/RealtimeUtils.kt
- chat-android/build.gradle.kts
- gradle/libs.versions.toml
- chat-android/src/test/java/com/ably/chat/integration/EmbeddedServer.kt
🔇 Additional comments (4)
chat-android/src/test/java/com/ably/chat/integration/MessagesIntegrationTest.kt (2)
3-3: Import added for BuildConfigThe new import for
BuildConfigis necessary for the new test case that verifies agent information in the channel parameters.
10-10: Import added for channelOptionsThis import is required to access channel options in the new test case.
chat-android/src/test/java/com/ably/chat/integration/RequestHeaderTest.kt (2)
1-15: New test file with appropriate importsThe imports cover all necessary dependencies for testing HTTP requests and Ably client functionality.
16-32: Test implementation for SDK agent trackingThis test effectively verifies that the SDK correctly includes the agent information in the HTTP headers, which aligns with the PR objective of enhancing SDK tracking.
The use of Turbine for testing asynchronous flows is a good practice. This allows for clean testing of the request headers without complex callback management.
To track Chat SDK usage, we replaced the old ably-java interfaces for Channel, Client, and Presence with new Kotlin-based interfaces. The new Kotlin interfaces include the
createWrapperSDKProxymethod, which injects agent information for the new set of agents.Summary by CodeRabbit