Add Tree concurrency tests #186
Conversation
WalkthroughThe recent updates introduce several key modifications: CI workflow adjustments to specify test classes, dependency version upgrades, consolidation of test annotations, and enhancements to JSON tree concurrency testing. Additionally, document status management in the Changes
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 (
|
7hong13
force-pushed
the
tree_concurrency_test
branch
from
75ad17e
to
9f86bbf
Compare
|
There was an issue with the following situation: It caused the TCs to keep failing. It is fixed in 286eca6. |
![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
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (14)
- .github/workflows/ci.yml (1 hunks)
- gradle/libs.versions.toml (1 hunks)
- yorkie/src/androidTest/kotlin/dev/yorkie/TestAnnotations.kt (1 hunks)
- yorkie/src/androidTest/kotlin/dev/yorkie/core/TestUtils.kt (1 hunks)
- yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeConcurrencyTest.kt (1 hunks)
- yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeConcurrencyTestHelpers.kt (1 hunks)
- yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeSplitMergeTest.kt (12 hunks)
- yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeTest.kt (2 hunks)
- yorkie/src/main/kotlin/dev/yorkie/core/Client.kt (2 hunks)
- yorkie/src/main/kotlin/dev/yorkie/document/Document.kt (2 hunks)
- yorkie/src/main/kotlin/dev/yorkie/document/json/JsonTree.kt (1 hunks)
- yorkie/src/test/kotlin/dev/yorkie/core/ClientTest.kt (1 hunks)
- yorkie/src/test/kotlin/dev/yorkie/document/DocumentTest.kt (3 hunks)
- yorkie/src/test/kotlin/dev/yorkie/document/json/JsonCounterTest.kt (3 hunks)
Files skipped from review due to trivial changes (5)
- gradle/libs.versions.toml
- yorkie/src/androidTest/kotlin/dev/yorkie/TestAnnotations.kt
- yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeTest.kt
- yorkie/src/test/kotlin/dev/yorkie/document/DocumentTest.kt
- yorkie/src/test/kotlin/dev/yorkie/document/json/JsonCounterTest.kt
Additional comments not posted (13)
yorkie/src/androidTest/kotlin/dev/yorkie/core/TestUtils.kt (1)
15-15: Verify server support for HTTP/2 protocol..github/workflows/ci.yml (1)
99-101: Updated CI configuration to use class-based test selection.This change should help in running specific tests more reliably, ensuring that only the relevant tests for each class are executed during CI builds.
yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeConcurrencyTestHelpers.kt (1)
1-231: Comprehensive addition of concurrency test helpers.The introduction of these helpers is crucial for robust concurrency testing of the Tree component. This will significantly aid in preventing concurrency-related issues and ensuring the integrity of operations under concurrent access.
yorkie/src/test/kotlin/dev/yorkie/core/ClientTest.kt (1)
78-213: Enhanced test coverage for Client operations.The modifications and additions to the test methods enhance coverage and robustness, ensuring that the Client class behaves as expected across various operations such as activation, deactivation, attachment, and synchronization.
yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeSplitMergeTest.kt (1)
Line range hint
36-231: Robust testing for JsonTree split and merge operations.The tests cover a wide range of scenarios, including edge cases and concurrent modifications, which are crucial for ensuring the correctness and robustness of the split and merge operations in the JsonTree.
yorkie/src/main/kotlin/dev/yorkie/document/json/JsonTree.kt (1)
134-145: Added overloaded edit method with split level support.The addition of this method enhances the flexibility and functionality of the JsonTree operations, allowing for more granular control over tree modifications, which is essential for certain types of edits.
yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeConcurrencyTest.kt (3)
329-358: Ensure proper client management in concurrency tests.Verification successful
The function
runTestConcurrencyinJsonTreeConcurrencyTest.ktcorrectly manages client activation and deactivation usingactivateAsyncanddeactivateAsync.
JsonTreeConcurrencyTest.ktlines 329-358: Proper client management is ensured withactivateAsyncanddeactivateAsync.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify proper client management in concurrency tests. # Test: Search for proper client activation and deactivation in concurrency tests. rg --type kotlin $'activateAsync|deactivateAsync'Length of output: 10534
213-325: Ensure proper synchronization and thread safety in concurrent tests involving style changes.
21-210: Ensure proper synchronization and thread safety in concurrent tests.yorkie/src/main/kotlin/dev/yorkie/document/Document.kt (2)
454-456: Properly encapsulated method for setting document status.
457-457: Garbage collection method correctly checks conditions before execution.yorkie/src/main/kotlin/dev/yorkie/core/Client.kt (2)
492-492: Correctly implemented method for setting client status during attachment.
538-538: Properly implemented method for setting client status during detachment.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- yorkie/src/main/kotlin/dev/yorkie/core/Client.kt (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- yorkie/src/main/kotlin/dev/yorkie/core/Client.kt
| @@ -12,7 +12,7 @@ const val GENERAL_TIMEOUT = 3_000L | |||
| fun createClient() = Client( | |||
| "http://10.0.2.2:8080", | |||
| unaryClient = OkHttpClient.Builder() | |||
| .protocols(listOf(Protocol.HTTP_1_1)) | |||
| .protocols(listOf(Protocol.H2_PRIOR_KNOWLEDGE)) | |||
There was a problem hiding this comment.
we should stay on http 1 as http 2 is very unstable
There was a problem hiding this comment.
using http2 is outdated code. I just tested it temporarily.
| @@ -451,6 +451,10 @@ public class Document( | |||
| onlineClients.value -= actorID | |||
| } | |||
|
|
|||
| internal fun setStatus(newStatus: DocumentStatus) { | |||
There was a problem hiding this comment.
do we need additional setter method for this?
Kotlin's properties are different from Java's fields, and it is not wrong to use properties for simple getter/setter's.
(https://blog.kotlin-academy.com/kotlin-should-i-define-function-or-property-6786951da909)
There was a problem hiding this comment.
I thought setting the Document.status from Client directly can violate the encapsulation. I reverted it based on your suggestion: 8febcf6
This reverts commit 9a2e440.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- yorkie/src/main/kotlin/dev/yorkie/core/Client.kt (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- yorkie/src/main/kotlin/dev/yorkie/core/Client.kt
What this PR does / why we need it?
Any background context you want to provide?
What are the relevant tickets?
Fixes #
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
androidxLifecycleandandroidx-annotationdependencies for enhanced performance and stability.Chores