Remove reattach test code #855
Conversation
Recently, when attaching a detached document, an ErrAlreadyDetached error was returned. This test code is no longer valid, so this commit removes it.
WalkthroughThe Changes
Poem
Tip Early access features: disabledWe are currently testing the following features in early access:
Note:
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #855 +/- ##
=======================================
Coverage 80.61% 80.61%
=======================================
Files 60 60
Lines 4565 4565
Branches 932 932
=======================================
Hits 3680 3680
Misses 616 616
Partials 269 269 ☔ View full report in Codecov by Sentry. |
![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 (5)
test/integration/document_test.ts (5)
Line range hint
207-208: Consider replacing thedeleteoperator for performance improvements.The
deleteoperator can lead to performance issues as it modifies the object's shape. Usingundefinedassignment is a recommended alternative.- delete root.obj.food; + root.obj.food = undefined;
Line range hint
121-150: Review of the test case 'Can watch documents'.The test case for watching document changes through subscriptions is well-structured. It properly initializes clients, attaches documents, and subscribes to document changes. The use of
EventCollectorto verify events is a robust method for testing asynchronous operations.Consider adding more assertions to ensure the state of documents after updates, especially when multiple clients are involved.
+ assert.equal(d1.toSortedJSON(), '{"k1":"v1"}', 'Verify d1 state after updates'); + assert.equal(d2.toSortedJSON(), '{"k1":"v1"}', 'Verify d2 state after updates');
Line range hint
181-210: Review of the test case 'specify the topic to subscribe to'.This test case demonstrates the use of targeted subscriptions to specific document paths. It's a good practice as it allows for more granular control and efficient testing of document changes.
Consider verifying that the unsubscribe operations are effective by checking that no further events are received after unsubscribing.
+ assert.equal(eventCollector.getLength(), 4, 'Verify no further events after unsubscribing');
Line range hint
241-270: Review of the test case 'Can handle tombstone'.The test case for handling tombstone scenarios is crucial for ensuring data integrity and proper cleanup. The use of sync operations to verify the state across clients is a good approach.
Recommend adding more assertions to explicitly verify the removal state of elements within documents.
+ assert.isTrue(prevArray?.isRemoved(), 'Verify that the previous array is marked as removed');
Line range hint
391-420: Review of the test case 'removed document removal test'.This test case seems to be a duplicate or very similar to previous tests involving document removal. Ensure that it provides unique value or consider integrating it with other related tests to avoid redundancy.
- // Consider removing or integrating this test with other document removal tests.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- test/integration/document_test.ts (1 hunks)
Additional context used
Biome
test/integration/document_test.ts
[error] 207-208: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
Additional comments not posted (10)
test/integration/document_test.ts (10)
Line range hint
101-120: Review of the test case 'Can remove document using removeIfNotAttached option when detaching'.This test case appears to be correctly handling the removal of documents based on attachment status. It's good to see comprehensive checks using assertions to confirm the document status after operations.
Ensure that the assertions cover all edge cases and the test descriptions accurately reflect the steps being tested.
Line range hint
151-180: Review of the test case 'detects the events from doc.subscribe'.This test case effectively captures and verifies document events. The detailed setup of operations and expected events helps in understanding the document's behavior under different operations.
Ensure that the test case descriptions are updated to reflect the new changes if any, and consider adding cleanup steps for the clients and documents to prevent resource leaks.
Line range hint
211-240: Review of the test case 'specify the nested topic to subscribe to'.This test case extends the previous one by applying subscriptions to nested document paths. It's well-implemented and covers a variety of operations including updates and deletions.
Ensure that the test case accurately captures and verifies the events for nested paths, and consider adding more detailed assertions for nested properties.
Line range hint
271-300: Review of the test case 'Can remove document'.This test case thoroughly checks the removal process of documents under various conditions. The use of
assertThrowsAsyncto expect specific errors is appropriate.Consider adding comments to explain the rationale behind each step, especially for complex scenarios involving multiple clients and document states.
Line range hint
301-330: Review of the test case 'Can create document with the same key as the removed document key'.This test explores the scenario of reusing document keys after removal. It's important for ensuring that document identifiers can be recycled without issues.
Ensure that the test covers scenarios where simultaneous operations on the same document key might lead to unexpected behaviors.
Line range hint
331-360: Review of the test case 'Can know that document has been removed when doing client.sync()'.This test case is essential for verifying that document removals are propagated correctly across clients. The detailed steps help in understanding the synchronization process.
Consider adding more detailed logging or debugging information to trace the sync process in complex scenarios.
Line range hint
361-390: Review of the test case 'Can know that document has been removed when doing client.detach()'.This test case checks the behavior of document status after detach operations, especially in scenarios involving multiple clients. It's well-structured and covers the necessary operations.
Recommend adding cleanup steps for clients to ensure they are properly deactivated after the test.
Line range hint
421-450: Review of the test case 'document state transition test'.This test case explores the state transitions of documents. It's crucial for ensuring that documents behave correctly across different states.
Recommend adding more comprehensive tests to cover transitions that might not be typical but are possible due to unusual user actions or system states.
Line range hint
451-480: Review of the 'Undo/Redo' test cases.These tests are vital for verifying the undo/redo functionality in document operations. The detailed checks and assertions provide good coverage.
Consider adding scenarios that test the limits of the undo/redo stack, such as undoing operations until the stack is empty, to ensure robustness.
Line range hint
11-100: Review of the test case 'Can attach/detach documents' after modifications.The test case for attaching and detaching documents seems to have been modified to include client deactivation but removes reattach steps. This aligns with the PR description that mentions the removal of reattach test code due to the introduction of
ErrAlreadyDetached.Ensure that the removal of these steps does not affect other dependent tests or functionalities.
Verification successful
Verification Successful: No other tests depend on the removed reattach steps, as no occurrences of "reattach" were found in the codebase.
- No dependent test cases were found.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that no other tests depend on the removed reattach steps. # Test: Search for dependent test cases. Expect: No occurrences of dependent test cases. rg --type typescript "reattach"Length of output: 66
Script:
#!/bin/bash # Description: Verify that no other tests depend on the removed reattach steps. # Test: Search for dependent test cases. Expect: No occurrences of dependent test cases. rg --glob '*.ts' "reattach"Length of output: 27
What this PR does / why we need it?
Remove reattach test code
Recently, when attaching a detached document, an ErrAlreadyDetached error was returned. This test code is no longer valid, so this commit removes it.
Related to yorkie-team/yorkie#904, yorkie-team/yorkie#908
Any background context you want to provide?
What are the relevant tickets?
Fixes #
Checklist
Summary by CodeRabbit