Skip to content
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

Reduce CI test time by optimizing task execution #988

Merged
merged 13 commits into from
Sep 11, 2024
Merged

Conversation

binary-ho
Copy link
Contributor

@binary-ho binary-ho commented Aug 31, 2024

1. What this PR does / why we need it:

Reduce CI Time and experiment with various methods


here is the existing ci time ('be335eb' commit ci run result)
image


Through experimentation, we could significantly reduce CI time to the level shown below,
but I decided not to adopt this approach.

image


I discarded various methods and, in the end, was able to reduce the time by just a little.

image

  • build : -1m 20s
  • bench : -1m
  • sharding-test : +1m



2. Which issue(s) this PR fixes:

Fixes #789

3. Implements

I tried various methods:

Some were actually adopted, while others were only experimented with and not adopted.

The ones adopted are marked as Adopt.

The ones not adopted are marked as Not Adopt, with reasons provided. Please review these and consider adopting them where appropriate.

  1. Adopt Remove unnecessary 'Get tools dependencies' step from bench, sharding jobs.
    • Reduced 1m 20s from bench and sharding-test jobs.
    • There is no disadvantage.

  2. Adopt Renamed sharding-test to complex-test and moved tree_concurrency_test to complex-test.
    It might be worth considering whether this change is beneficial.
    • Reduced 1m 20s from 2m.
    • Disadvantage : But complex-test execution time increased by 2m.
    • Expanded target packages for complex-test. While the original sharding-test only required testing changes in server/backend/database/**, now changes in pkg/document/** and client/** packages also trigger tests.

  3. Not Adopt Run integration test in parallel.
    • Reduced 1m 20s from build job.
    • Disadvantage : There are variables shared for integration tests that have concurrent write operations, causing tests to fail due to the -race option. Preventing race conditions now is easy, but it cannot be guaranteed for the future. This requires more consideration. Further details are provided below.

  4. Not Adopt Cache make tools results.
    • Reduced 1m 20s from build job.
    • Disadvantage: Caching the result of go install is easy, but utilizing it is inconvenient, and it is prone to break if the make tools process changes. More details are provided below.

For a more convenient review, the detailed content is collapsed. Click to expand and read the details.



3.1 Removing Unnecessary Dependency Step

The dependencies installed by make tools are not needed in bench and sharding-test jobs.

Therefore, I removed the unnecessary 'Get tools dependencies' steps from the bench and sharding jobs.

image

This reduces 1m 20s in bench and sharding-test jobs.

The bench only reduced by 1m 20s. The 3m 20s in the previous CI might look less due to caching of the bench results in the previous CI, where caching time is not displayed.



3.2 Parallel Test in Integration Package

Applying parallel tests to the integration package can reduce 1m 20s from the build job.

However, I did not apply it.

Here are the reasons why
  1. The current integration tests have shared states for test environment setups, and some tests perform writes to these shared states. Therefore, race conditions occur, and the tests fail due to Yorkie's -race test option.

  2. It is possible to prevent these race conditions by removing shared writes or using methods like Go's write mutexes that prevent simultaneous writes.

    Eventually, I believe these shared writes should be managed. The purpose of the -race option is to validate race conditions in the code being tested, not in the test setup process.

    However, even after removing shared writes, other issues remain and require further thought.

    1. When writing tests in the integration package after introducing parallel tests, contributors must consider these race conditions.
    2. Even if considered, it is challenging to be certain that there are no race conditions. Potential race conditions do not always fail tests. In my experience, the same code only caused build test failures about once every 20-30 times.

    Therefore, it is not possible to know upfront if a test writer avoided race conditions correctly. I am exploring a test approach to prevent this situation, and I plan to share it if I find a solution.

    Once these issues are resolved, we could consider applying parallel tests to Yorkie's longer-running tests, potentially reducing CI or test times significantly. (Benchmarks could also be reduced)

Therefore, I have removed the parallel test introduced earlier.

If test times get longer in the future, adopting it might be worth considering.



3.3 Reasons for Not Caching make tools

All CI jobs initially had a step to call make tools. Therefore, I attempted to cache the results of make tools, achieving savings of 1m 20s in all jobs.

When the cache hits, the make-tools job execution time in the capture below is less than 10 seconds.

image

Check the test results. after make-tools cache result

However, I decided not to adopt caching for make tools.

Here are the reasons
  • Advantages:

    • Saves 1m 20s in the build test when the cache hits.
  • Disadvantages:

    • The compressed cache takes up about 250MB of space. Also, this cache needs to be managed separately. GitHub Actions deletes caches not accessed for more than 30 days. However, the GitHub Actions Free Plan only provides 500MB.
      You could add steps to manage this separately, but this complicates the CI process.
    • It is difficult to utilize caching.
      If a dependency installation other than go install is added to the make tools process later, the cache target must be changed.

    Even if go install finds the output files, calling it still takes a significant amount of time. From my tests, even if the results of make tools are pre-installed in $GOPATH/bin and $GOPATH/pkg, it still takes about 1 minute, not much different from the original 1m 20s.

    Removing the make tools step would allow you to take advantage of caching. Currently, make tools only calls go install 5 times. However, if dependency installation methods other than go install are added later, additional folder caching must be included in the CI.

    • Caching could result in at least one job and numerous steps being added to the YAML file.
      This significantly affects the readability of the YAML file. If caching is used, the file would look like this:
      ci.yml after make-tools cache



3.4 Moving tree_concurrency_test to complex-test

As proposed in the issue, tree_concurrency_test was moved to sharding-test,
and some common types and methods were moved to main_test.go.

Additionally, sharding-test was renamed to complex-test.

result:
image

Advantages:

  1. Reduces build CI time by 1m 20s.
    (If parallel tests are introduced, CI time will not decrease)

Disadvantages:

  1. After moving, the scope of the target for complex-test is broadened, so it runs more frequently.
  2. complex-test time increases by about 2m.
  3. The name complex-test feels a bit ambiguous (to me).

3.4.1 Broadened Target Scope for complex-test

As-Is

Currently, sharding-test is only triggered by changes in server/backend/database/**.

image

After Move

After the move, the target package for complex-test expands, causing it to run more often!

image

Summary by CodeRabbit

  • New Features

    • Introduced a new testing framework focusing on complex scenarios, enhancing the scope of functionality being tested.
    • Added a comprehensive suite of unit tests for both SDK and Admin RPC server backends with sharded databases.
  • Bug Fixes

    • Improved organization and clarity of test results through the introduction of a new struct for assertions.
  • Refactor

    • Renamed and reorganized several test functions and packages to better reflect their purpose and improve code maintainability.
  • Chores

    • Simplified CI workflow by updating job names and paths for testing, streamlining the testing process.

@binary-ho binary-ho requested a review from krapie August 31, 2024 18:17
@binary-ho binary-ho self-assigned this Aug 31, 2024
Copy link

coderabbitai bot commented Aug 31, 2024

Walkthrough

The changes involve significant updates to the CI workflow and testing framework, including renaming the sharding-test job to complex-test, modifying test paths, and introducing new test structures. The package names have been updated from sharding to complex, reflecting a broader scope of functionality. Additionally, new functions were added to enhance modularity, and the organization of tests was improved for better clarity and efficiency.

Changes

Files Change Summary
.github/workflows/ci.yml Renamed sharding-test job to complex-test, updated job outputs and conditions, modified test paths.
test/complex/main_test.go Changed package name from sharding to complex, introduced clientAndDocPair struct, and added new functions for client management.
test/complex/mongo_client_test.go Changed package name from sharding to complex, reflecting new context for tests.
test/complex/server_test.go Introduced new unit tests for SDK and Admin RPC server backends with a focus on sharded databases.
test/complex/tree_concurrency_test.go Changed package name from integration to complex, indicating a shift in test categorization.
test/integration/main_test.go Introduced testResult struct, modified functions for handling assertions and test results.

Sequence Diagram(s)

sequenceDiagram
    participant CI as CI Workflow
    participant CT as Complex Test
    participant IT as Integration Test

    CI->>CT: Trigger Complex Test
    CT->>CT: Run Tests
    CT->>IT: Run Integration Tests
    IT->>IT: Execute Assertions
    IT->>CI: Return Results
Loading

Assessment against linked issues

Objective Addressed Explanation
Rename "Sharding Test" to "Complex Test" (789)
Move "Tree Concurrency Test" to "Complex Test" (789) The Tree Concurrency Test is not moved.

Possibly related PRs

  • Fine-tune CI Workflows in PR #964: The changes in this PR also involve significant modifications to the CI workflow in the .github/workflows/ci.yml file, focusing on optimizing job execution based on file changes, which aligns with the renaming and restructuring of jobs in the main PR.

Poem

🐰 In the meadow where the bunnies play,
A new test hops in, brightening the day.
Complex and clever, it dances with glee,
Renamed and refined, as happy as can be!
With clients in sync, all troubles take flight,
Hooray for the changes, everything feels right! 🌼


Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between aa9e6b4 and 6546db1.

Files selected for processing (3)
  • test/complex/main_test.go (4 hunks)
  • test/complex/tree_concurrency_test.go (3 hunks)
  • test/integration/main_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
  • test/complex/tree_concurrency_test.go
  • test/integration/main_test.go
Additional comments not posted (6)
test/complex/main_test.go (6)

19-19: Package renaming looks good!

The package name change from sharding to complex accurately reflects the broader scope of functionality being tested in this file.


44-47: testResult struct definition looks good!

The testResult struct is well-defined with descriptive field names. It seems to be used for storing test results, which is a good practice for organizing test data.


49-52: clientAndDocPair struct definition looks good!

The clientAndDocPair struct is well-defined with descriptive field names. It encapsulates a client and a document, which enhances the organization of client-document pairs used in the tests.


145-173: syncClientsThenCheckEqual function looks good!

The function effectively consolidates the logic to synchronize clients and check document consistency, streamlining the testing process.

The previous suggestion to add more detailed logging or error messages has been addressed in the current implementation, which has sufficient logging and error handling.


177-186: activeClients function looks good!

The function enhances the modularity and reusability of the code by encapsulating the logic to create and activate multiple clients. It is well-implemented and uses appropriate error handling.


189-193: deactivateAndCloseClients function looks good!

The function enhances the modularity and reusability of the code by encapsulating the logic to deactivate and close multiple clients. It is well-implemented and uses appropriate error handling.


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

codecov bot commented Aug 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 50.69%. Comparing base (3e49afb) to head (6546db1).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #988      +/-   ##
==========================================
- Coverage   50.80%   50.69%   -0.12%     
==========================================
  Files          73       74       +1     
  Lines       10865    10901      +36     
==========================================
+ Hits         5520     5526       +6     
- Misses       4796     4826      +30     
  Partials      549      549              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go Benchmark

Benchmark suite Current: 1a7c968 Previous: be335eb Ratio
BenchmarkDocument/constructor_test 1496 ns/op 1337 B/op 24 allocs/op 1482 ns/op 1337 B/op 24 allocs/op 1.01
BenchmarkDocument/constructor_test - ns/op 1496 ns/op 1482 ns/op 1.01
BenchmarkDocument/constructor_test - B/op 1337 B/op 1337 B/op 1
BenchmarkDocument/constructor_test - allocs/op 24 allocs/op 24 allocs/op 1
BenchmarkDocument/status_test 1103 ns/op 1305 B/op 22 allocs/op 942 ns/op 1305 B/op 22 allocs/op 1.17
BenchmarkDocument/status_test - ns/op 1103 ns/op 942 ns/op 1.17
BenchmarkDocument/status_test - B/op 1305 B/op 1305 B/op 1
BenchmarkDocument/status_test - allocs/op 22 allocs/op 22 allocs/op 1
BenchmarkDocument/equals_test 7603 ns/op 7273 B/op 132 allocs/op 8294 ns/op 7273 B/op 132 allocs/op 0.92
BenchmarkDocument/equals_test - ns/op 7603 ns/op 8294 ns/op 0.92
BenchmarkDocument/equals_test - B/op 7273 B/op 7273 B/op 1
BenchmarkDocument/equals_test - allocs/op 132 allocs/op 132 allocs/op 1
BenchmarkDocument/nested_update_test 16671 ns/op 12139 B/op 262 allocs/op 17491 ns/op 12139 B/op 262 allocs/op 0.95
BenchmarkDocument/nested_update_test - ns/op 16671 ns/op 17491 ns/op 0.95
BenchmarkDocument/nested_update_test - B/op 12139 B/op 12139 B/op 1
BenchmarkDocument/nested_update_test - allocs/op 262 allocs/op 262 allocs/op 1
BenchmarkDocument/delete_test 22368 ns/op 15364 B/op 341 allocs/op 22594 ns/op 15363 B/op 341 allocs/op 0.99
BenchmarkDocument/delete_test - ns/op 22368 ns/op 22594 ns/op 0.99
BenchmarkDocument/delete_test - B/op 15364 B/op 15363 B/op 1.00
BenchmarkDocument/delete_test - allocs/op 341 allocs/op 341 allocs/op 1
BenchmarkDocument/object_test 8653 ns/op 6817 B/op 120 allocs/op 8600 ns/op 6817 B/op 120 allocs/op 1.01
BenchmarkDocument/object_test - ns/op 8653 ns/op 8600 ns/op 1.01
BenchmarkDocument/object_test - B/op 6817 B/op 6817 B/op 1
BenchmarkDocument/object_test - allocs/op 120 allocs/op 120 allocs/op 1
BenchmarkDocument/array_test 28985 ns/op 11947 B/op 276 allocs/op 29208 ns/op 11947 B/op 276 allocs/op 0.99
BenchmarkDocument/array_test - ns/op 28985 ns/op 29208 ns/op 0.99
BenchmarkDocument/array_test - B/op 11947 B/op 11947 B/op 1
BenchmarkDocument/array_test - allocs/op 276 allocs/op 276 allocs/op 1
BenchmarkDocument/text_test 30501 ns/op 14715 B/op 469 allocs/op 30350 ns/op 14715 B/op 469 allocs/op 1.00
BenchmarkDocument/text_test - ns/op 30501 ns/op 30350 ns/op 1.00
BenchmarkDocument/text_test - B/op 14715 B/op 14715 B/op 1
BenchmarkDocument/text_test - allocs/op 469 allocs/op 469 allocs/op 1
BenchmarkDocument/text_composition_test 29008 ns/op 18422 B/op 484 allocs/op 28754 ns/op 18422 B/op 484 allocs/op 1.01
BenchmarkDocument/text_composition_test - ns/op 29008 ns/op 28754 ns/op 1.01
BenchmarkDocument/text_composition_test - B/op 18422 B/op 18422 B/op 1
BenchmarkDocument/text_composition_test - allocs/op 484 allocs/op 484 allocs/op 1
BenchmarkDocument/rich_text_test 81048 ns/op 38476 B/op 1148 allocs/op 80222 ns/op 38476 B/op 1148 allocs/op 1.01
BenchmarkDocument/rich_text_test - ns/op 81048 ns/op 80222 ns/op 1.01
BenchmarkDocument/rich_text_test - B/op 38476 B/op 38476 B/op 1
BenchmarkDocument/rich_text_test - allocs/op 1148 allocs/op 1148 allocs/op 1
BenchmarkDocument/counter_test 17309 ns/op 10722 B/op 244 allocs/op 17272 ns/op 10722 B/op 244 allocs/op 1.00
BenchmarkDocument/counter_test - ns/op 17309 ns/op 17272 ns/op 1.00
BenchmarkDocument/counter_test - B/op 10722 B/op 10722 B/op 1
BenchmarkDocument/counter_test - allocs/op 244 allocs/op 244 allocs/op 1
BenchmarkDocument/text_edit_gc_100 1275150 ns/op 871016 B/op 16753 allocs/op 1281277 ns/op 870959 B/op 16753 allocs/op 1.00
BenchmarkDocument/text_edit_gc_100 - ns/op 1275150 ns/op 1281277 ns/op 1.00
BenchmarkDocument/text_edit_gc_100 - B/op 871016 B/op 870959 B/op 1.00
BenchmarkDocument/text_edit_gc_100 - allocs/op 16753 allocs/op 16753 allocs/op 1
BenchmarkDocument/text_edit_gc_1000 49746467 ns/op 50537032 B/op 181713 allocs/op 49571799 ns/op 50535357 B/op 181718 allocs/op 1.00
BenchmarkDocument/text_edit_gc_1000 - ns/op 49746467 ns/op 49571799 ns/op 1.00
BenchmarkDocument/text_edit_gc_1000 - B/op 50537032 B/op 50535357 B/op 1.00
BenchmarkDocument/text_edit_gc_1000 - allocs/op 181713 allocs/op 181718 allocs/op 1.00
BenchmarkDocument/text_split_gc_100 1857731 ns/op 1528824 B/op 15606 allocs/op 1931395 ns/op 1528814 B/op 15605 allocs/op 0.96
BenchmarkDocument/text_split_gc_100 - ns/op 1857731 ns/op 1931395 ns/op 0.96
BenchmarkDocument/text_split_gc_100 - B/op 1528824 B/op 1528814 B/op 1.00
BenchmarkDocument/text_split_gc_100 - allocs/op 15606 allocs/op 15605 allocs/op 1.00
BenchmarkDocument/text_split_gc_1000 110607616 ns/op 135079305 B/op 182204 allocs/op 111436929 ns/op 135078137 B/op 182198 allocs/op 0.99
BenchmarkDocument/text_split_gc_1000 - ns/op 110607616 ns/op 111436929 ns/op 0.99
BenchmarkDocument/text_split_gc_1000 - B/op 135079305 B/op 135078137 B/op 1.00
BenchmarkDocument/text_split_gc_1000 - allocs/op 182204 allocs/op 182198 allocs/op 1.00
BenchmarkDocument/text_delete_all_10000 15689213 ns/op 10183151 B/op 40676 allocs/op 15350743 ns/op 10183702 B/op 40677 allocs/op 1.02
BenchmarkDocument/text_delete_all_10000 - ns/op 15689213 ns/op 15350743 ns/op 1.02
BenchmarkDocument/text_delete_all_10000 - B/op 10183151 B/op 10183702 B/op 1.00
BenchmarkDocument/text_delete_all_10000 - allocs/op 40676 allocs/op 40677 allocs/op 1.00
BenchmarkDocument/text_delete_all_100000 289047462 ns/op 142682596 B/op 411683 allocs/op 273939806 ns/op 142692892 B/op 411730 allocs/op 1.06
BenchmarkDocument/text_delete_all_100000 - ns/op 289047462 ns/op 273939806 ns/op 1.06
BenchmarkDocument/text_delete_all_100000 - B/op 142682596 B/op 142692892 B/op 1.00
BenchmarkDocument/text_delete_all_100000 - allocs/op 411683 allocs/op 411730 allocs/op 1.00
BenchmarkDocument/text_100 215392 ns/op 120035 B/op 5081 allocs/op 217232 ns/op 120037 B/op 5081 allocs/op 0.99
BenchmarkDocument/text_100 - ns/op 215392 ns/op 217232 ns/op 0.99
BenchmarkDocument/text_100 - B/op 120035 B/op 120037 B/op 1.00
BenchmarkDocument/text_100 - allocs/op 5081 allocs/op 5081 allocs/op 1
BenchmarkDocument/text_1000 2330262 ns/op 1169008 B/op 50085 allocs/op 2368040 ns/op 1169023 B/op 50085 allocs/op 0.98
BenchmarkDocument/text_1000 - ns/op 2330262 ns/op 2368040 ns/op 0.98
BenchmarkDocument/text_1000 - B/op 1169008 B/op 1169023 B/op 1.00
BenchmarkDocument/text_1000 - allocs/op 50085 allocs/op 50085 allocs/op 1
BenchmarkDocument/array_1000 1235152 ns/op 1091304 B/op 11831 allocs/op 1223537 ns/op 1091421 B/op 11832 allocs/op 1.01
BenchmarkDocument/array_1000 - ns/op 1235152 ns/op 1223537 ns/op 1.01
BenchmarkDocument/array_1000 - B/op 1091304 B/op 1091421 B/op 1.00
BenchmarkDocument/array_1000 - allocs/op 11831 allocs/op 11832 allocs/op 1.00
BenchmarkDocument/array_10000 13061523 ns/op 9799314 B/op 120294 allocs/op 13145794 ns/op 9799181 B/op 120293 allocs/op 0.99
BenchmarkDocument/array_10000 - ns/op 13061523 ns/op 13145794 ns/op 0.99
BenchmarkDocument/array_10000 - B/op 9799314 B/op 9799181 B/op 1.00
BenchmarkDocument/array_10000 - allocs/op 120294 allocs/op 120293 allocs/op 1.00
BenchmarkDocument/array_gc_100 151150 ns/op 132724 B/op 1260 allocs/op 150221 ns/op 132726 B/op 1261 allocs/op 1.01
BenchmarkDocument/array_gc_100 - ns/op 151150 ns/op 150221 ns/op 1.01
BenchmarkDocument/array_gc_100 - B/op 132724 B/op 132726 B/op 1.00
BenchmarkDocument/array_gc_100 - allocs/op 1260 allocs/op 1261 allocs/op 1.00
BenchmarkDocument/array_gc_1000 1409753 ns/op 1159182 B/op 12877 allocs/op 1404039 ns/op 1159265 B/op 12877 allocs/op 1.00
BenchmarkDocument/array_gc_1000 - ns/op 1409753 ns/op 1404039 ns/op 1.00
BenchmarkDocument/array_gc_1000 - B/op 1159182 B/op 1159265 B/op 1.00
BenchmarkDocument/array_gc_1000 - allocs/op 12877 allocs/op 12877 allocs/op 1
BenchmarkDocument/counter_1000 203849 ns/op 193081 B/op 5771 allocs/op 200699 ns/op 193080 B/op 5771 allocs/op 1.02
BenchmarkDocument/counter_1000 - ns/op 203849 ns/op 200699 ns/op 1.02
BenchmarkDocument/counter_1000 - B/op 193081 B/op 193080 B/op 1.00
BenchmarkDocument/counter_1000 - allocs/op 5771 allocs/op 5771 allocs/op 1
BenchmarkDocument/counter_10000 2168279 ns/op 2088011 B/op 59778 allocs/op 2153316 ns/op 2087996 B/op 59778 allocs/op 1.01
BenchmarkDocument/counter_10000 - ns/op 2168279 ns/op 2153316 ns/op 1.01
BenchmarkDocument/counter_10000 - B/op 2088011 B/op 2087996 B/op 1.00
BenchmarkDocument/counter_10000 - allocs/op 59778 allocs/op 59778 allocs/op 1
BenchmarkDocument/object_1000 1393853 ns/op 1428106 B/op 9849 allocs/op 1389200 ns/op 1428101 B/op 9849 allocs/op 1.00
BenchmarkDocument/object_1000 - ns/op 1393853 ns/op 1389200 ns/op 1.00
BenchmarkDocument/object_1000 - B/op 1428106 B/op 1428101 B/op 1.00
BenchmarkDocument/object_1000 - allocs/op 9849 allocs/op 9849 allocs/op 1
BenchmarkDocument/object_10000 15431983 ns/op 12168393 B/op 100570 allocs/op 15251274 ns/op 12165752 B/op 100560 allocs/op 1.01
BenchmarkDocument/object_10000 - ns/op 15431983 ns/op 15251274 ns/op 1.01
BenchmarkDocument/object_10000 - B/op 12168393 B/op 12165752 B/op 1.00
BenchmarkDocument/object_10000 - allocs/op 100570 allocs/op 100560 allocs/op 1.00
BenchmarkDocument/tree_100 1057534 ns/op 943696 B/op 6101 allocs/op 1067082 ns/op 943702 B/op 6101 allocs/op 0.99
BenchmarkDocument/tree_100 - ns/op 1057534 ns/op 1067082 ns/op 0.99
BenchmarkDocument/tree_100 - B/op 943696 B/op 943702 B/op 1.00
BenchmarkDocument/tree_100 - allocs/op 6101 allocs/op 6101 allocs/op 1
BenchmarkDocument/tree_1000 76521083 ns/op 86460296 B/op 60114 allocs/op 78296747 ns/op 86460417 B/op 60114 allocs/op 0.98
BenchmarkDocument/tree_1000 - ns/op 76521083 ns/op 78296747 ns/op 0.98
BenchmarkDocument/tree_1000 - B/op 86460296 B/op 86460417 B/op 1.00
BenchmarkDocument/tree_1000 - allocs/op 60114 allocs/op 60114 allocs/op 1
BenchmarkDocument/tree_10000 9485198577 ns/op 8580659728 B/op 600209 allocs/op 9552784024 ns/op 8580668816 B/op 600218 allocs/op 0.99
BenchmarkDocument/tree_10000 - ns/op 9485198577 ns/op 9552784024 ns/op 0.99
BenchmarkDocument/tree_10000 - B/op 8580659728 B/op 8580668816 B/op 1.00
BenchmarkDocument/tree_10000 - allocs/op 600209 allocs/op 600218 allocs/op 1.00
BenchmarkDocument/tree_delete_all_1000 77578944 ns/op 87509553 B/op 75264 allocs/op 76041256 ns/op 87509533 B/op 75265 allocs/op 1.02
BenchmarkDocument/tree_delete_all_1000 - ns/op 77578944 ns/op 76041256 ns/op 1.02
BenchmarkDocument/tree_delete_all_1000 - B/op 87509553 B/op 87509533 B/op 1.00
BenchmarkDocument/tree_delete_all_1000 - allocs/op 75264 allocs/op 75265 allocs/op 1.00
BenchmarkDocument/tree_edit_gc_100 3868938 ns/op 4146762 B/op 15141 allocs/op 3817038 ns/op 4146787 B/op 15141 allocs/op 1.01
BenchmarkDocument/tree_edit_gc_100 - ns/op 3868938 ns/op 3817038 ns/op 1.01
BenchmarkDocument/tree_edit_gc_100 - B/op 4146762 B/op 4146787 B/op 1.00
BenchmarkDocument/tree_edit_gc_100 - allocs/op 15141 allocs/op 15141 allocs/op 1
BenchmarkDocument/tree_edit_gc_1000 303492400 ns/op 383743376 B/op 154848 allocs/op 299657163 ns/op 383745182 B/op 154841 allocs/op 1.01
BenchmarkDocument/tree_edit_gc_1000 - ns/op 303492400 ns/op 299657163 ns/op 1.01
BenchmarkDocument/tree_edit_gc_1000 - B/op 383743376 B/op 383745182 B/op 1.00
BenchmarkDocument/tree_edit_gc_1000 - allocs/op 154848 allocs/op 154841 allocs/op 1.00
BenchmarkDocument/tree_split_gc_100 2553835 ns/op 2412421 B/op 11125 allocs/op 2526823 ns/op 2412451 B/op 11125 allocs/op 1.01
BenchmarkDocument/tree_split_gc_100 - ns/op 2553835 ns/op 2526823 ns/op 1.01
BenchmarkDocument/tree_split_gc_100 - B/op 2412421 B/op 2412451 B/op 1.00
BenchmarkDocument/tree_split_gc_100 - allocs/op 11125 allocs/op 11125 allocs/op 1
BenchmarkDocument/tree_split_gc_1000 184632879 ns/op 222251680 B/op 121998 allocs/op 181837078 ns/op 222252034 B/op 121995 allocs/op 1.02
BenchmarkDocument/tree_split_gc_1000 - ns/op 184632879 ns/op 181837078 ns/op 1.02
BenchmarkDocument/tree_split_gc_1000 - B/op 222251680 B/op 222252034 B/op 1.00
BenchmarkDocument/tree_split_gc_1000 - allocs/op 121998 allocs/op 121995 allocs/op 1.00
BenchmarkRPC/client_to_server 346252908 ns/op 16271498 B/op 167371 allocs/op 379803088 ns/op 17302797 B/op 175790 allocs/op 0.91
BenchmarkRPC/client_to_server - ns/op 346252908 ns/op 379803088 ns/op 0.91
BenchmarkRPC/client_to_server - B/op 16271498 B/op 17302797 B/op 0.94
BenchmarkRPC/client_to_server - allocs/op 167371 allocs/op 175790 allocs/op 0.95
BenchmarkRPC/client_to_client_via_server 632274346 ns/op 32632928 B/op 320838 allocs/op 629763409 ns/op 35103884 B/op 321005 allocs/op 1.00
BenchmarkRPC/client_to_client_via_server - ns/op 632274346 ns/op 629763409 ns/op 1.00
BenchmarkRPC/client_to_client_via_server - B/op 32632928 B/op 35103884 B/op 0.93
BenchmarkRPC/client_to_client_via_server - allocs/op 320838 allocs/op 321005 allocs/op 1.00
BenchmarkRPC/attach_large_document 1296446640 ns/op 1919095048 B/op 8854 allocs/op 1232019596 ns/op 1895482152 B/op 8842 allocs/op 1.05
BenchmarkRPC/attach_large_document - ns/op 1296446640 ns/op 1232019596 ns/op 1.05
BenchmarkRPC/attach_large_document - B/op 1919095048 B/op 1895482152 B/op 1.01
BenchmarkRPC/attach_large_document - allocs/op 8854 allocs/op 8842 allocs/op 1.00
BenchmarkRPC/adminCli_to_server 549092658 ns/op 35960224 B/op 289592 allocs/op 555831948 ns/op 35958556 B/op 289557 allocs/op 0.99
BenchmarkRPC/adminCli_to_server - ns/op 549092658 ns/op 555831948 ns/op 0.99
BenchmarkRPC/adminCli_to_server - B/op 35960224 B/op 35958556 B/op 1.00
BenchmarkRPC/adminCli_to_server - allocs/op 289592 allocs/op 289557 allocs/op 1.00
BenchmarkLocker 63.92 ns/op 16 B/op 1 allocs/op 64.93 ns/op 16 B/op 1 allocs/op 0.98
BenchmarkLocker - ns/op 63.92 ns/op 64.93 ns/op 0.98
BenchmarkLocker - B/op 16 B/op 16 B/op 1
BenchmarkLocker - allocs/op 1 allocs/op 1 allocs/op 1
BenchmarkLockerParallel 39.25 ns/op 0 B/op 0 allocs/op 38.44 ns/op 0 B/op 0 allocs/op 1.02
BenchmarkLockerParallel - ns/op 39.25 ns/op 38.44 ns/op 1.02
BenchmarkLockerParallel - B/op 0 B/op 0 B/op 1
BenchmarkLockerParallel - allocs/op 0 allocs/op 0 allocs/op 1
BenchmarkLockerMoreKeys 145.8 ns/op 15 B/op 0 allocs/op 154.1 ns/op 15 B/op 0 allocs/op 0.95
BenchmarkLockerMoreKeys - ns/op 145.8 ns/op 154.1 ns/op 0.95
BenchmarkLockerMoreKeys - B/op 15 B/op 15 B/op 1
BenchmarkLockerMoreKeys - allocs/op 0 allocs/op 0 allocs/op 1
BenchmarkChange/Push_10_Changes 3636183 ns/op 116205 B/op 1205 allocs/op 3934286 ns/op 121883 B/op 1286 allocs/op 0.92
BenchmarkChange/Push_10_Changes - ns/op 3636183 ns/op 3934286 ns/op 0.92
BenchmarkChange/Push_10_Changes - B/op 116205 B/op 121883 B/op 0.95
BenchmarkChange/Push_10_Changes - allocs/op 1205 allocs/op 1286 allocs/op 0.94
BenchmarkChange/Push_100_Changes 14397413 ns/op 569719 B/op 6576 allocs/op 14749095 ns/op 573554 B/op 6657 allocs/op 0.98
BenchmarkChange/Push_100_Changes - ns/op 14397413 ns/op 14749095 ns/op 0.98
BenchmarkChange/Push_100_Changes - B/op 569719 B/op 573554 B/op 0.99
BenchmarkChange/Push_100_Changes - allocs/op 6576 allocs/op 6657 allocs/op 0.99
BenchmarkChange/Push_1000_Changes 118048570 ns/op 5345807 B/op 63069 allocs/op 118523129 ns/op 5381654 B/op 63150 allocs/op 1.00
BenchmarkChange/Push_1000_Changes - ns/op 118048570 ns/op 118523129 ns/op 1.00
BenchmarkChange/Push_1000_Changes - B/op 5345807 B/op 5381654 B/op 0.99
BenchmarkChange/Push_1000_Changes - allocs/op 63069 allocs/op 63150 allocs/op 1.00
BenchmarkChange/Pull_10_Changes 2943591 ns/op 101498 B/op 1009 allocs/op 2926736 ns/op 101438 B/op 1008 allocs/op 1.01
BenchmarkChange/Pull_10_Changes - ns/op 2943591 ns/op 2926736 ns/op 1.01
BenchmarkChange/Pull_10_Changes - B/op 101498 B/op 101438 B/op 1.00
BenchmarkChange/Pull_10_Changes - allocs/op 1009 allocs/op 1008 allocs/op 1.00
BenchmarkChange/Pull_100_Changes 4433095 ns/op 266821 B/op 3479 allocs/op 4414953 ns/op 266780 B/op 3479 allocs/op 1.00
BenchmarkChange/Pull_100_Changes - ns/op 4433095 ns/op 4414953 ns/op 1.00
BenchmarkChange/Pull_100_Changes - B/op 266821 B/op 266780 B/op 1.00
BenchmarkChange/Pull_100_Changes - allocs/op 3479 allocs/op 3479 allocs/op 1
BenchmarkChange/Pull_1000_Changes 8628956 ns/op 1491268 B/op 29860 allocs/op 8713124 ns/op 1491591 B/op 29859 allocs/op 0.99
BenchmarkChange/Pull_1000_Changes - ns/op 8628956 ns/op 8713124 ns/op 0.99
BenchmarkChange/Pull_1000_Changes - B/op 1491268 B/op 1491591 B/op 1.00
BenchmarkChange/Pull_1000_Changes - allocs/op 29860 allocs/op 29859 allocs/op 1.00
BenchmarkSnapshot/Push_3KB_snapshot 16906830 ns/op 706061 B/op 6576 allocs/op 17160075 ns/op 710046 B/op 6657 allocs/op 0.99
BenchmarkSnapshot/Push_3KB_snapshot - ns/op 16906830 ns/op 17160075 ns/op 0.99
BenchmarkSnapshot/Push_3KB_snapshot - B/op 706061 B/op 710046 B/op 0.99
BenchmarkSnapshot/Push_3KB_snapshot - allocs/op 6576 allocs/op 6657 allocs/op 0.99
BenchmarkSnapshot/Push_30KB_snapshot 120121127 ns/op 5750715 B/op 63135 allocs/op 121877878 ns/op 5654650 B/op 63152 allocs/op 0.99
BenchmarkSnapshot/Push_30KB_snapshot - ns/op 120121127 ns/op 121877878 ns/op 0.99
BenchmarkSnapshot/Push_30KB_snapshot - B/op 5750715 B/op 5654650 B/op 1.02
BenchmarkSnapshot/Push_30KB_snapshot - allocs/op 63135 allocs/op 63152 allocs/op 1.00
BenchmarkSnapshot/Pull_3KB_snapshot 6514689 ns/op 922747 B/op 15515 allocs/op 6586946 ns/op 922936 B/op 15516 allocs/op 0.99
BenchmarkSnapshot/Pull_3KB_snapshot - ns/op 6514689 ns/op 6586946 ns/op 0.99
BenchmarkSnapshot/Pull_3KB_snapshot - B/op 922747 B/op 922936 B/op 1.00
BenchmarkSnapshot/Pull_3KB_snapshot - allocs/op 15515 allocs/op 15516 allocs/op 1.00
BenchmarkSnapshot/Pull_30KB_snapshot 15196517 ns/op 7157278 B/op 150109 allocs/op 15778481 ns/op 7157978 B/op 150110 allocs/op 0.96
BenchmarkSnapshot/Pull_30KB_snapshot - ns/op 15196517 ns/op 15778481 ns/op 0.96
BenchmarkSnapshot/Pull_30KB_snapshot - B/op 7157278 B/op 7157978 B/op 1.00
BenchmarkSnapshot/Pull_30KB_snapshot - allocs/op 150109 allocs/op 150110 allocs/op 1.00
BenchmarkSync/memory_sync_10_test 6924 ns/op 1286 B/op 38 allocs/op 7948 ns/op 1286 B/op 38 allocs/op 0.87
BenchmarkSync/memory_sync_10_test - ns/op 6924 ns/op 7948 ns/op 0.87
BenchmarkSync/memory_sync_10_test - B/op 1286 B/op 1286 B/op 1
BenchmarkSync/memory_sync_10_test - allocs/op 38 allocs/op 38 allocs/op 1
BenchmarkSync/memory_sync_100_test 51419 ns/op 8636 B/op 273 allocs/op 48167 ns/op 9019 B/op 297 allocs/op 1.07
BenchmarkSync/memory_sync_100_test - ns/op 51419 ns/op 48167 ns/op 1.07
BenchmarkSync/memory_sync_100_test - B/op 8636 B/op 9019 B/op 0.96
BenchmarkSync/memory_sync_100_test - allocs/op 273 allocs/op 297 allocs/op 0.92
BenchmarkSync/memory_sync_1000_test 584935 ns/op 74304 B/op 2120 allocs/op 469400 ns/op 80282 B/op 2493 allocs/op 1.25
BenchmarkSync/memory_sync_1000_test - ns/op 584935 ns/op 469400 ns/op 1.25
BenchmarkSync/memory_sync_1000_test - B/op 74304 B/op 80282 B/op 0.93
BenchmarkSync/memory_sync_1000_test - allocs/op 2120 allocs/op 2493 allocs/op 0.85
BenchmarkSync/memory_sync_10000_test 7031902 ns/op 736247 B/op 20295 allocs/op 7096307 ns/op 738328 B/op 20325 allocs/op 0.99
BenchmarkSync/memory_sync_10000_test - ns/op 7031902 ns/op 7096307 ns/op 0.99
BenchmarkSync/memory_sync_10000_test - B/op 736247 B/op 738328 B/op 1.00
BenchmarkSync/memory_sync_10000_test - allocs/op 20295 allocs/op 20325 allocs/op 1.00
BenchmarkTextEditing 5287290122 ns/op 3901893568 B/op 18743144 allocs/op 4982109025 ns/op 3901900544 B/op 18743138 allocs/op 1.06
BenchmarkTextEditing - ns/op 5287290122 ns/op 4982109025 ns/op 1.06
BenchmarkTextEditing - B/op 3901893568 B/op 3901900544 B/op 1.00
BenchmarkTextEditing - allocs/op 18743144 allocs/op 18743138 allocs/op 1.00
BenchmarkTree/10000_vertices_to_protobuf 3465780 ns/op 6262972 B/op 70025 allocs/op 3469513 ns/op 6262990 B/op 70025 allocs/op 1.00
BenchmarkTree/10000_vertices_to_protobuf - ns/op 3465780 ns/op 3469513 ns/op 1.00
BenchmarkTree/10000_vertices_to_protobuf - B/op 6262972 B/op 6262990 B/op 1.00
BenchmarkTree/10000_vertices_to_protobuf - allocs/op 70025 allocs/op 70025 allocs/op 1
BenchmarkTree/10000_vertices_from_protobuf 154057809 ns/op 442171409 B/op 290038 allocs/op 151684384 ns/op 442171322 B/op 290038 allocs/op 1.02
BenchmarkTree/10000_vertices_from_protobuf - ns/op 154057809 ns/op 151684384 ns/op 1.02
BenchmarkTree/10000_vertices_from_protobuf - B/op 442171409 B/op 442171322 B/op 1.00
BenchmarkTree/10000_vertices_from_protobuf - allocs/op 290038 allocs/op 290038 allocs/op 1
BenchmarkTree/20000_vertices_to_protobuf 8230186 ns/op 12717031 B/op 140028 allocs/op 7623663 ns/op 12716916 B/op 140028 allocs/op 1.08
BenchmarkTree/20000_vertices_to_protobuf - ns/op 8230186 ns/op 7623663 ns/op 1.08
BenchmarkTree/20000_vertices_to_protobuf - B/op 12717031 B/op 12716916 B/op 1.00
BenchmarkTree/20000_vertices_to_protobuf - allocs/op 140028 allocs/op 140028 allocs/op 1
BenchmarkTree/20000_vertices_from_protobuf 686926018 ns/op 1697272560 B/op 580089 allocs/op 678280684 ns/op 1697272504 B/op 580089 allocs/op 1.01
BenchmarkTree/20000_vertices_from_protobuf - ns/op 686926018 ns/op 678280684 ns/op 1.01
BenchmarkTree/20000_vertices_from_protobuf - B/op 1697272560 B/op 1697272504 B/op 1.00
BenchmarkTree/20000_vertices_from_protobuf - allocs/op 580089 allocs/op 580089 allocs/op 1
BenchmarkTree/30000_vertices_to_protobuf 12051026 ns/op 19318326 B/op 210030 allocs/op 11823398 ns/op 19318330 B/op 210030 allocs/op 1.02
BenchmarkTree/30000_vertices_to_protobuf - ns/op 12051026 ns/op 11823398 ns/op 1.02
BenchmarkTree/30000_vertices_to_protobuf - B/op 19318326 B/op 19318330 B/op 1.00
BenchmarkTree/30000_vertices_to_protobuf - allocs/op 210030 allocs/op 210030 allocs/op 1
BenchmarkTree/30000_vertices_from_protobuf 1642091998 ns/op 3752052552 B/op 870050 allocs/op 1623720069 ns/op 3752052968 B/op 870053 allocs/op 1.01
BenchmarkTree/30000_vertices_from_protobuf - ns/op 1642091998 ns/op 1623720069 ns/op 1.01
BenchmarkTree/30000_vertices_from_protobuf - B/op 3752052552 B/op 3752052968 B/op 1.00
BenchmarkTree/30000_vertices_from_protobuf - allocs/op 870050 allocs/op 870053 allocs/op 1.00

This comment was automatically generated by workflow using github-action-benchmark.

@krapie krapie added the enhancement 🌟 New feature or request label Sep 2, 2024
@hackerwins hackerwins force-pushed the main branch 2 times, most recently from 8eb4284 to b77c25a Compare September 5, 2024 08:36
@krapie
Copy link
Member

krapie commented Sep 5, 2024

@binary-ho Could you rebase your commit to current upstream? There were some changes in previous commit.

@binary-ho binary-ho marked this pull request as ready for review September 7, 2024 17:02
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Outside diff range, codebase verification and nitpick comments (2)
test/complex/mongo_client_test.go (2)

Line range hint 36-52: Consider Using Configuration Management for MongoDB Settings

The function setupMongoClient uses hardcoded values for MongoDB connection settings. Consider using environment variables or a configuration file to manage these settings, enhancing flexibility and security.


Line range hint 54-118: Rename Function to Align with New Naming Convention

The function TestClientWithShardedDB should be renamed to reflect the new complex naming convention, perhaps to TestClientWithComplexDB. The tests within the function are well-structured and effectively use assertions to validate operations.

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 3e49afb and aa9e6b4.

Files selected for processing (6)
  • .github/workflows/ci.yml (6 hunks)
  • test/complex/main_test.go (4 hunks)
  • test/complex/mongo_client_test.go (2 hunks)
  • test/complex/server_test.go (1 hunks)
  • test/complex/tree_concurrency_test.go (2 hunks)
  • test/integration/main_test.go (4 hunks)
Additional comments not posted (13)
test/complex/server_test.go (2)

27-59: Well-structured test functions for SDK RPC server backend.

The tests are well-organized and each subtest is clearly named, which improves readability and maintainability. Consider adding a comment to explain the setup or source of testClient used in the tests, especially if it's not immediately obvious from the file context.


61-105: Well-structured test functions for Admin RPC server backend.

The tests are consistently structured and clearly named, enhancing both readability and maintainability. It would be beneficial to add comments explaining the setup or source of testAdminClient and testAdminAuthInterceptor, especially if these are not defined within the file.

test/integration/main_test.go (3)

37-40: New type testResult enhances test outcome clarity.

The addition of testResult is a good practice as it standardizes how test outcomes are reported, making the tests more maintainable and the results easier to understand.


Line range hint 69-97: Refactored function syncClientsThenCheckEqual improves flexibility.

The modification to return a boolean value instead of directly asserting within the function allows for more flexible test result handling. Consider adding documentation to explain the return values and when to use this function over others.


Line range hint 100-123: Refactored function syncClientsThenAssertEqual simplifies test assertions.

Reverting to direct assertions within the function simplifies its usage and makes the test code cleaner. Documentation on when to use this function compared to its counterpart that returns a boolean would be helpful.

.github/workflows/ci.yml (1)

26-26: Significant updates to the complex-test job enhance test coverage.

The renaming of the job from sharding-test to complex-test and the updates to the test paths are well-aligned with the PR's objectives to reflect a broader scope of functionality. However, the removal of the "Get tools dependencies" step should be carefully evaluated for any potential impact on the build process.

Consider verifying the impact of removing the "Get tools dependencies" step to ensure it does not affect the build's stability or completeness.

Also applies to: 45-48, 138-177

test/complex/mongo_client_test.go (2)

1-1: Build Tag and Package Name Update Approved

The update from sharding to complex in both the build tag and package declaration aligns with the PR's objectives to reflect a broader scope of functionality. This change is correctly implemented.

Also applies to: 19-19


Line range hint 120-168: Comprehensive Test for Duplicate Document IDs

The function FindDocInfoByRefKey with duplicate ID test is well-documented and effectively tests the handling of duplicate document IDs in MongoDB. The use of assertions ensures that each step of the test is correctly validated.

test/complex/main_test.go (4)

19-19: Package Name Change Approved

The change from package sharding to package complex is consistent with the PR objectives to better reflect the broader scope of functionality.


44-47: New Struct clientAndDocPair Approved

The introduction of clientAndDocPair enhances modularity and organization by encapsulating client-document pairs, which is crucial for the new testing functions.


171-181: Function activeClients Approved

This function effectively centralizes the creation and activation of client instances, enhancing modularity and reusability.


183-188: Function deactivateAndCloseClients Approved

The function properly manages the deactivation and closure of client instances, which is essential for resource management and cleanup in tests.

test/complex/tree_concurrency_test.go (1)

19-19: Package Name Change Approved

The change from package integration to package complex aligns with the PR objectives and reflects the shift in testing strategy towards more complex scenarios.

Comment on lines +140 to +168
func syncClientsThenCheckEqual(t *testing.T, pairs []clientAndDocPair) bool {
assert.True(t, len(pairs) > 1)
ctx := context.Background()
// Save own changes and get previous changes.
for i, pair := range pairs {
fmt.Printf("before d%d: %s\n", i+1, pair.doc.Marshal())
err := pair.cli.Sync(ctx)
assert.NoError(t, err)
}

t.Run("remove document test", func(t *testing.T) {
testcases.RunRemoveDocumentTest(t, testClient)
})
// Get last client changes.
// Last client get all precede changes in above loop.
for _, pair := range pairs[:len(pairs)-1] {
err := pair.cli.Sync(ctx)
assert.NoError(t, err)
}

t.Run("remove document with invalid client state test", func(t *testing.T) {
testcases.RunRemoveDocumentWithInvalidClientStateTest(t, testClient)
})
// Assert start.
expected := pairs[0].doc.Marshal()
fmt.Printf("after d1: %s\n", expected)
for i, pair := range pairs[1:] {
v := pair.doc.Marshal()
fmt.Printf("after d%d: %s\n", i+2, v)
if expected != v {
return false
}
}

t.Run("watch document test", func(t *testing.T) {
testcases.RunWatchDocumentTest(t, testClient)
})
return true
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function syncClientsThenCheckEqual Approved with Suggestion

The function effectively consolidates logic to synchronize clients and check document consistency. However, consider adding more detailed logging or error messages for better debuggability in case of synchronization failures.

Copy link
Member

@krapie krapie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution.

I understand that this PR modifies two things. If my understanding is incorrect, please let me know.

  1. Remove unnecessary make tools in CI workflow
  2. Rename sharding-test to complex-text with moving tree_concurrency_test.go in complex-text, as @hackerwins suggested.

Seems like your work has reduced the CI time about 1 min, and I think this is huge 👍🏼
Considering that we are only running specific CI task like complex-test on specific file changes, this will synergy with that CI behavior.

Leaving my small curiously:

Is there any reason for modifying test/integration/main_test.go? It seems like you switched function order and created testResult struct which seems to be originated from tree_concurrency_test.go and seems this struct is not being used?

@binary-ho
Copy link
Contributor Author

binary-ho commented Sep 11, 2024

Thank you for review @krapie !
also, sorry for not providing more context about testResult.

testResult type was defined in concurrent_tree_test but was also being used in array_test.
Therefore, i could't leave the testResult type in concurrent_tree_test
it needed to stay within the integration package.


image


However, since the testResult type was used in multiple tests within the integration package
and could potentially be used in the future as well, I decided to place it in test_main instead of array_test.

If you feel that placing the testResult type in array_test.go would be more appropriate according to principles like YANGI, please let me know.

@binary-ho binary-ho requested a review from krapie September 11, 2024 08:31
@krapie
Copy link
Member

krapie commented Sep 11, 2024

@binary-ho Then how about moving testResult struct in test/complex/tree_concurrency_test.go to test/complex/main_test.go, just like integration package?

Also I will understand switched function order as to keep consistency between two main_test.go files in integration and complex package.

@binary-ho
Copy link
Contributor Author

binary-ho commented Sep 11, 2024

you're right.. why didn’t I think of that?
I have moved the testResult of the complex package to main_test.go

And the change in the function order was my mistake....
I thought I had checked it, but I missed it

Copy link
Member

@krapie krapie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 🌟 New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Reduce CI test time by optimizing task execution
2 participants