Fix tests for event batch publisher implementation #924
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
packages/sdk/test/integration/presence_test.ts (3)
129-132: Consider improving type safety of presence assertions.The assertions correctly verify the initial presence states. However, we can improve type safety by creating a type for the presence array structure.
type PresenceEntry = { clientID: string; presence: { name: string }; }; assert.deepEqual( deepSort<PresenceEntry>(doc1.getPresences()), deepSort<PresenceEntry>([ { clientID: c1ID, presence: { name: 'a' } } ]) );Also applies to: 139-145
Line range hint
471-477: Consider making the test more deterministic.The comment indicates a timing dependency between realtime sync and watch stream resolution. While the current workaround (performing changeSyncMode after sync) works, it might make the test fragile.
Consider these improvements:
- Extract the timing-dependent logic into a helper function that ensures deterministic behavior
- Add retry logic with timeout for event verification
async function ensureSyncAndModeChange(client: Client, doc: Document) { await client.sync(); await new Promise(resolve => setTimeout(resolve, 100)); // Add small delay await client.changeSyncMode(doc, SyncMode.Realtime); // Add retry logic for event verification const maxRetries = 3; for (let i = 0; i < maxRetries; i++) { try { await events.waitAndVerifyNthEvent(/* ... */); break; } catch (err) { if (i === maxRetries - 1) throw err; await new Promise(resolve => setTimeout(resolve, 100)); } } }🧰 Tools
🪛 Biome
[error] 134-134: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
Line range hint
590-677: Consider adding edge cases to undo/redo tests.The tests cover basic undo/redo functionality well. Consider adding these edge cases:
- Maximum undo stack size behavior
- Concurrent presence updates from multiple clients
- Undo/redo behavior during network disconnection
Example test case:
it('Should handle undo stack size limits correctly', async function ({ task }) { type Presence = { color: string }; const doc = new yorkie.Document<{}, Presence>(docKey); await client.attach(doc, { initialPresence: { color: 'red' } }); // Fill undo stack to limit for (let i = 0; i < 100; i++) { doc.update((root, presence) => { presence.set({ color: `color${i}` }, { addToHistory: true }); }); } // Verify oldest entries are removed assert.isBelow(doc.getUndoStackForTest().length, 100); });🧰 Tools
🪛 Biome
[error] 134-134: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
packages/sdk/test/integration/client_test.ts (1)
758-761: Consider adding error handling for document attachmentWhile the test verifies basic attachment functionality, it would be good to add explicit error handling for attachment failures.
- await c1.attach(d1); - await c2.attach(d2); - await eventCollectorD1.waitAndVerifyNthEvent(1, DocumentSyncStatus.Synced); + try { + await c1.attach(d1); + await c2.attach(d2); + await eventCollectorD1.waitAndVerifyNthEvent(1, DocumentSyncStatus.Synced); + } catch (err) { + assert.fail(`Document attachment failed: ${err.message}`); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/sdk/src/document/document.ts(1 hunks)packages/sdk/test/integration/client_test.ts(10 hunks)packages/sdk/test/integration/presence_test.ts(1 hunks)
🧰 Additional context used
🪛 Biome
packages/sdk/test/integration/presence_test.ts
[error] 134-134: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🔇 Additional comments (8)
packages/sdk/test/integration/presence_test.ts (1)
134-134: 🛠️ Refactor suggestion
Replace empty object type with explicit interface.
The static analysis tool correctly flags the use of {} as a type. This can lead to type safety issues.
-deepSort([{ clientID: InitialActorID, presence: {} }])
+interface EmptyPresence {}
+deepSort([{ clientID: InitialActorID, presence: {} as EmptyPresence }])Likely invalid or redundant comment.
🧰 Tools
🪛 Biome
[error] 134-134: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
packages/sdk/test/integration/client_test.ts (6)
379-380: LGTM: Proper handling of sync mode changes
The addition of await for changeSyncMode calls ensures proper synchronization when switching between different sync modes.
400-401: LGTM: Consistent async handling
Consistent use of await for sync mode changes maintains proper test sequencing.
422-423: LGTM: Proper restoration of realtime sync
The test correctly verifies that switching back to realtime sync mode properly restores bidirectional synchronization.
772-772: LGTM: Push-only mode sync optimization
The test correctly verifies that unnecessary syncs are avoided in push-only mode and proper sync occurs when switching back to realtime mode.
Also applies to: 783-783
624-624: LGTM: Push-only mode verification
The test properly verifies that remote changes are prevented in push-only mode and restored when switching back to realtime mode.
Also applies to: 638-638
✅ Verification successful
Based on the search results, I can now verify the review comment. The codebase shows consistent handling of push-only mode across test files and implementation. Specifically:
-
The test in
client_test.tsproperly verifies push-only mode behavior by:- Testing the transition to push-only mode
- Verifying that remote changes are prevented
- Checking restoration of sync when switching back to realtime mode
-
The implementation in
attachment.tscorrectly handles push-only mode by:- Only allowing local changes to be pushed (
hasLocalChanges()check) - Preventing remote changes from being processed
- Only allowing local changes to be pushed (
-
The client implementation in
client.tsproperly manages the mode transitions and sync behavior.
LGTM: Push-only mode implementation is verified
The test correctly validates the push-only mode behavior, and the implementation consistently handles this mode across the codebase. The synchronization restrictions are properly enforced in both the client and attachment layers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistent handling of push-only mode across the codebase
ast-grep --pattern 'changeSyncMode($_, SyncMode.RealtimePushOnly)'
Length of output: 68
Script:
#!/bin/bash
# Let's try a broader search for changeSyncMode usage and SyncMode enum
rg "changeSyncMode" -A 2
rg "SyncMode\." -A 2
Length of output: 39306
707-707: LGTM: Sync-off mode verification
The test properly verifies that remote changes are prevented in sync-off mode and restored when switching back to realtime mode.
Also applies to: 721-721
✅ Verification successful
Based on the code analysis, I can now provide a final response:
Verified: Sync-off mode properly prevents remote changes
The test verifies that:
- When in RealtimeSyncOff mode, the client does not receive remote changes
- Remote changes are properly received after switching back to Realtime mode
- The implementation correctly handles sync mode transitions
The test Should prevent remote changes in sync-off mode specifically validates this behavior by:
- Setting up two clients with realtime sync
- Changing one client to sync-off mode
- Verifying no remote changes are received while in sync-off mode
- Confirming changes are received after switching back to realtime mode
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistent handling of sync-off mode across the codebase
ast-grep --pattern 'changeSyncMode($_, SyncMode.RealtimeSyncOff)'
Length of output: 67
Script:
#!/bin/bash
# Let's try a broader search for sync mode related code
rg "SyncMode\." -A 2 -B 2
# Also search for changeSyncMode method
rg "changeSyncMode" -A 2 -B 2
# And look for the test file content to understand the test context
cat packages/sdk/test/integration/client_test.ts
Length of output: 97432
packages/sdk/src/document/document.ts (1)
1603-1606: LGTM! Well-structured condition for preventing duplicate event processing.
The added condition effectively prevents duplicate processing of watched events by checking if the client is already tracked and has presence data. This change aligns well with the PR's objective of fixing the event batch publisher implementation and ensures proper event delivery sequencing.
What this PR does / why we need it?
Fix tests that were broken after introducing the event batch publisher
- Only current subscribers received events
- Events are delivered based on subscription status at batch publish time
1. Fix watched event delivery to late subscribers
With batched delivery, watched events could be sent to clients who subscribed after the initial watch event occurred.
2. Update attach event test sequencing
The change in event delivery timing affected how attach and subscribe events work:
Any background context you want to provide?
What are the relevant tickets?
Fixes #
Related yorkie-team/yorkie#1052
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests