-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
LiveObjects SYNC sequence tests #1894
base: integration/liveobjects
Are you sure you want to change the base?
Conversation
ccb1a06
to
0cdc7c7
Compare
6a56b3e
to
3fc1df0
Compare
3fc1df0
to
3ee12cd
Compare
WalkthroughThe changes in this pull request introduce several new functionalities and modifications across multiple files. A new Changes
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (4)
src/plugins/liveobjects/livemap.ts (1)
63-65
: LGTM! Consider addingreadonly
modifier.The implementation of the
size()
method looks good. It correctly returns the size of the internalMap
object, which is consistent with the class structure and provides a useful accessor for the map's size.As a minor enhancement, consider adding the
readonly
modifier to the method signature:readonly size(): number { return this._dataRef.data.size; }This would explicitly indicate that the method doesn't modify the object's state, which can be helpful for TypeScript users and potentially enable certain optimizations.
test/common/modules/testapp_manager.js (1)
136-136
: LGTM! Consider adding a comment for clarity.The addition of the 'enableChannelState' feature flag to the
post_apps
object is straightforward and appears to be intentional. This change will affect the configuration of newly created test apps, potentially enabling a new feature or behavior in the test environment.Consider adding a brief comment explaining the purpose of this feature flag for future maintainers. For example:
// Enable channel state tracking for test apps testData.post_apps.featureFlags = ['enableChannelState'];This will help other developers understand the intention behind this configuration.
test/realtime/live_objects.test.js (2)
109-110
: Ensure client is properly closed after test completionAt lines 109-110, a client is created but not explicitly closed after the test finishes:
const client = RealtimeWithLiveObjects(helper);Ensure that the client is properly closed to prevent resource leaks. You can wrap the test logic with
helper.monitorConnectionThenCloseAndFinish
to manage the client lifecycle, which you've done correctly in other tests.
383-384
: Clarify assertion message for nested counter's valueIn lines 383-384, the assertion checks the value of a nested counter:
expect(counterFromReferencedMap.value()).to.equal(20, 'Check nested counter has correct value');Consider enhancing the assertion message to specify which counter is being checked for clarity:
- expect(counterFromReferencedMap.value()).to.equal(20, 'Check nested counter has correct value'); + expect(counterFromReferencedMap.value()).to.equal(20, 'Check that "counterFromReferencedMap" has the correct value');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- src/plugins/liveobjects/livemap.ts (1 hunks)
- test/common/globals/named_dependencies.js (1 hunks)
- test/common/modules/live_objects_helper.js (1 hunks)
- test/common/modules/testapp_manager.js (1 hunks)
- test/realtime/live_objects.test.js (3 hunks)
🧰 Additional context used
🪛 Biome
test/common/modules/live_objects_helper.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
[error] 171-171: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (2)
test/common/globals/named_dependencies.js (1)
29-32
: LGTM! New dependency added correctly.The new
live_objects_helper
entry has been added consistently with the existing pattern. Both browser and Node.js paths are provided, which is in line with the structure of other entries in this file.To ensure the correctness of the paths and the existence of the referenced module, please run the following verification script:
✅ Verification successful
Verification Successful:
live_objects_helper
module exists and paths are correct.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the live_objects_helper module # Test: Check if the file exists at the specified path if [ -f "test/common/modules/live_objects_helper.js" ]; then echo "live_objects_helper.js file found at the specified path." else echo "Error: live_objects_helper.js file not found at the specified path." exit 1 fi # Test: Search for any imports or requires of the new module echo "Searching for usage of live_objects_helper in the codebase:" rg --type js "require\(['\"]test/common/modules/live_objects_helper['\"]" || echo "No usage found. This might be expected if the module is not yet used in tests."Length of output: 554
src/plugins/liveobjects/livemap.ts (1)
63-65
: Well-integrated addition to the LiveMap classThe
size()
method is a valuable addition to theLiveMap
class. It's well-placed within the class structure and provides functionality that users would expect from a map-like object. This method enhances the usability of theLiveMap
class by allowing easy access to the size of the underlying map, which is a common operation in many use cases.The addition of this method aligns the
LiveMap
interface more closely with the standard JavaScriptMap
interface, which is a positive design choice for API consistency and user expectations.
3ee12cd
to
a64b155
Compare
a64b155
to
1510c81
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
test/realtime/live_objects.test.js (1)
123-138
: LGTM: Verification of root object identificationThis test case is a good addition:
- It confirms that getRoot() returns a live object with the expected id "root".
- It ensures the correct identification of the root object in the LiveObjects structure.
Note: The test uses a private API call (getObjectId()). While this is acceptable for testing, ensure that this method remains consistent across versions to maintain test stability.
Consider adding a comment explaining the use of the private API call, to prevent future confusion or unintended changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- src/plugins/liveobjects/livemap.ts (1 hunks)
- test/common/globals/named_dependencies.js (1 hunks)
- test/common/modules/live_objects_helper.js (1 hunks)
- test/common/modules/testapp_manager.js (1 hunks)
- test/realtime/live_objects.test.js (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/plugins/liveobjects/livemap.ts
- test/common/globals/named_dependencies.js
- test/common/modules/testapp_manager.js
🧰 Additional context used
🪛 Biome
test/common/modules/live_objects_helper.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
[error] 170-170: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (17)
test/common/modules/live_objects_helper.js (7)
3-6
: LGTM: Clear module purpose and structureThe comment clearly describes the purpose of the LiveObjectsHelper, and the module definition with its dependency is correctly structured.
7-17
: LGTM: Well-defined constants and utility functionThe ACTIONS constant clearly defines the available operations, and the nonce function provides a simple way to generate unique identifiers. This structure enhances code readability and maintainability.
80-91
: LGTM: Well-structured helper methodThe
_createAndSetOnMap
method is well-structured and correctly handles the creation and setting of objects on maps. The use of separate state requests for creation and setting ensures atomicity of operations.
93-127
: LGTM: Well-structured operation creation methodsThe
_mapCreateOp
and_mapSetOp
methods are well-structured and correctly use the ACTIONS constant to specify the action type. They provide a clear interface for creating map operations.
146-167
: LGTM: Robust state request handlingThe
_stateRequest
method is well-implemented. It correctly handles single object state requests, processes the response appropriately, and includes robust error handling. The extraction of the object ID from the response is a nice touch for convenience.
1-171
: Overall assessment: Well-implemented helper with room for minor improvementsThe
LiveObjectsHelper
class is well-implemented and achieves its purpose of creating and managing LiveObjects. The code is generally well-structured and includes appropriate error handling.Key strengths:
- Clear definition of actions and operations
- Robust state request handling
- Comprehensive implementation of the required state tree
Suggestions for improvement:
- Remove the redundant 'use strict' directive
- Refactor the
initForChannel
method for better readability- Ensure proper handling of zero count in counter creation
- Simplify the module export statement
These improvements will enhance the maintainability and robustness of the code. Great job overall!
🧰 Tools
🪛 Biome
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
[error] 170-170: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
1-1
:⚠️ Potential issueRemove redundant 'use strict' directive
The 'use strict' directive is unnecessary in ES6 modules as they are strict by default. You can safely remove this line.
Apply this diff to remove the redundant directive:
-'use strict';
Likely invalid or redundant comment.
🧰 Tools
🪛 Biome
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
test/realtime/live_objects.test.js (10)
3-25
: LGTM: Improved code organization and reusabilityThe changes in this section enhance the code structure:
- Adding 'live_objects_helper' import improves modularity.
- Renaming 'LiveObjectsRealtime' to 'RealtimeWithLiveObjects' increases clarity.
- The new 'channelOptionsWithLiveObjects' function improves code reusability.
These modifications contribute to better maintainability of the test suite.
54-95
: LGTM: Robust test for system behavior without LiveObjects pluginThis new test case is a valuable addition:
- It verifies that the system doesn't break when receiving STATE_SYNC ProtocolMessages without the LiveObjects plugin.
- It ensures that regular message subscriptions continue to function correctly.
This test enhances the overall robustness of the system by covering an important edge case.
100-106
: LGTM: Verification of LiveObjects plugin integrationThis test case is a good addition:
- It confirms that the 'liveObjects' property returns a LiveObjects class instance when the plugin is properly integrated.
- It uses the renamed 'RealtimeWithLiveObjects' function, maintaining consistency with earlier changes.
This test helps ensure the correct setup and integration of the LiveObjects plugin.
107-121
: LGTM: Verification of getRoot() functionalityThis test case is a valuable addition:
- It confirms that getRoot() returns a LiveMap instance as expected.
- It uses the new channelOptionsWithLiveObjects() function, promoting consistency in channel setup across tests.
This test helps ensure the correct behavior of a core LiveObjects functionality.
140-154
: LGTM: Verification of empty root initializationThis test case is a valuable addition:
- It confirms that getRoot() returns an empty root object when no state exists on a channel.
- It verifies the correct initialization of LiveObjects on an empty channel.
This test helps ensure proper behavior in scenarios where no initial state is present, which is crucial for maintaining consistency in the LiveObjects system.
156-182
: LGTM: Verification of getRoot() asynchronous behaviorThis test case is an excellent addition:
- It confirms that getRoot() waits for the initial STATE_SYNC to complete before resolving.
- It uses nextTick to verify that getRoot() doesn't resolve prematurely.
This test is crucial for ensuring the correct asynchronous behavior of getRoot(), which is essential for maintaining data consistency in the LiveObjects system.
184-208
: LGTM: Verification of efficient getRoot() behavior post-synchronizationThis test case is a valuable addition:
- It confirms that getRoot() resolves immediately when the STATE_SYNC sequence is completed.
- It uses nextTick to verify the timing of the resolution, ensuring it happens on the next tick after synchronization.
This test is important for ensuring efficient behavior of getRoot() after the initial synchronization, which can impact the performance of applications using LiveObjects.
210-286
: LGTM: Comprehensive verification of getRoot() behavior during re-synchronizationThis test case is an excellent addition:
- It confirms that getRoot() waits for subsequent STATE_SYNC sequences to finish before resolving.
- It simulates a new synchronization sequence by injecting STATE_SYNC messages, providing a realistic test scenario.
- It verifies the correct state of the root object after the new synchronization sequence.
This test is crucial for ensuring the reliability of LiveObjects during re-synchronization scenarios, which can occur in real-world applications due to network issues or other factors.
288-447
: LGTM: Comprehensive testing of LiveObjects state building and initializationThese test cases are excellent additions:
- They verify the correct building of the state object tree from the STATE_SYNC sequence.
- They ensure proper initialization of LiveCounter and LiveMap with initial values.
- They confirm that LiveMaps can reference the same object in their keys.
These tests are crucial for ensuring the correct behavior and structure of LiveObjects state in various scenarios. They provide comprehensive coverage of the LiveObjects functionality, which is essential for maintaining the reliability and consistency of the system.
450-464
: 🛠️ Refactor suggestionConsider using RealtimeWithLiveObjects for comprehensive testing
While the test logic is correct, there's an opportunity for improvement:
- The test currently uses
helper.AblyRealtime()
instead ofRealtimeWithLiveObjects(helper)
.- A previous comment suggested using
RealtimeWithLiveObjects
for more comprehensive testing of LiveObjects functionality.To ensure full coverage of LiveObjects functionality, consider updating the test as follows:
- const client = helper.AblyRealtime(); + const client = RealtimeWithLiveObjects(helper);This change will ensure that the test is using the Realtime client with the LiveObjects plugin, providing a more comprehensive test of the LiveObjects state modes.
4780a99
to
42c9d5c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
test/realtime/live_objects.test.js (1)
450-464
: Consider using RealtimeWithLiveObjects for comprehensive testingThis test case effectively verifies that a channel can be attached with LiveObjects state modes. However, as suggested in a previous comment, consider updating the test to use
RealtimeWithLiveObjects(helper)
instead ofhelper.AblyRealtime()
. This change would ensure that the test covers the full LiveObjects functionality.Here's how you could modify the test:
- const client = helper.AblyRealtime(); + const client = RealtimeWithLiveObjects(helper);This modification would provide more comprehensive coverage of the LiveObjects functionality in this test case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- test/realtime/live_objects.test.js (3 hunks)
🧰 Additional context used
🔇 Additional comments (8)
test/realtime/live_objects.test.js (8)
3-25
: Improved code organization and readabilityThe changes in this section enhance the overall structure of the test file:
- Addition of 'live_objects_helper' import
- Renaming 'LiveObjectsRealtime' to 'RealtimeWithLiveObjects'
- Introduction of 'channelOptionsWithLiveObjects' function
These modifications make the code more descriptive and easier to understand.
54-95
: Good addition: Handling STATE_SYNC without LiveObjects pluginThis new test case is a valuable addition. It ensures that the system remains stable when receiving STATE_SYNC ProtocolMessages, even without the LiveObjects plugin. This improves the robustness of the system and prevents potential issues in mixed environments.
100-121
: Comprehensive basic LiveObjects functionality testsThese new test cases provide good coverage for the basic functionality of LiveObjects:
- Verifying the
liveObjects
property on the channel- Checking that
getRoot()
returns a LiveMap instanceThe consistent use of the renamed
RealtimeWithLiveObjects
function is noted and appreciated.
123-286
: Thorough coverage of getRoot() scenariosThe new test cases for
getRoot()
are comprehensive and well-structured. They cover important scenarios such as:
- Returning an empty root when no state exists
- Waiting for initial STATE_SYNC to complete
- Resolving immediately when STATE_SYNC is already completed
- Handling subsequent STATE_SYNC messages
These tests significantly improve the reliability and robustness of the LiveObjects functionality.
288-339
: Comprehensive state object tree building testThis test case thoroughly verifies the correct building of the state object tree from the STATE_SYNC sequence. It checks:
- The correct number of keys in the root object
- The existence and correct types of counters and maps
- The correct number of keys in nested maps
This level of detail ensures that the state synchronization process is working as expected.
341-363
: Thorough LiveCounter initialization testThis test case effectively verifies the correct initialization of LiveCounter objects with values from the STATE_SYNC sequence. It covers multiple scenarios:
- Empty counter
- Counter with initial value
- Referenced counter
This ensures that LiveCounter objects are properly synchronized across different use cases.
365-409
: Comprehensive LiveMap initialization testThis test case provides excellent coverage for LiveMap initialization:
- Verifies empty map initialization
- Checks referenced map with nested counter
- Tests various data types (string, empty string, bytes, empty bytes, number, zero, boolean)
- Verifies nested map structures
The thoroughness of this test ensures robust handling of different data types and structures in LiveMap objects.
411-447
: Effective test for LiveMap object referencesThis test case effectively verifies that LiveMaps can reference the same object in different parts of the structure:
- Checks that a nested counter in a map is the same instance as a counter on the root
- Verifies that a nested map is the same instance as a map on the root
This ensures consistency and proper object referencing within the LiveObjects structure.
d3e15d9
to
d637bd7
Compare
42c9d5c
to
566bf00
Compare
This PR is based on #1891, please review it first.
Adds LiveObjects integration tests for functionality introduced in #1887, #1890 and #1891.
NOTE: next Live Objects tests are expected to fail until https://ably.atlassian.net/browse/DTP-982 is fixed:
Failure is due to missing state object in STATE_SYNC sequence for a map which has zero values.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
size()
method to theLiveMap
class for retrieving the size of the data map.live_objects_helper
dependency for testing.LiveObjectsHelper
class to manage LiveObjects state on channels.enableChannelState
for new app creation.Bug Fixes
Documentation