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

Add unit test of multipler orders for multiple terms aggregation. #13400

Conversation

Zhikai-VM
Copy link
Contributor

@Zhikai-VM Zhikai-VM commented Apr 26, 2024

Description

I added unit tests to verify the multiple orders in the multiple terms aggregation.

Related Issues

Resolves #13375

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Failing checks are inspected and point to the corresponding known issue(s) (See: Troubleshooting Failing Builds)
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)
  • Public documentation issue/PR created

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Copy link
Contributor

❕ Gradle check result for f09ee2d: UNSTABLE

  • TEST FAILURES:
      2 org.opensearch.common.util.concurrent.QueueResizableOpenSearchThreadPoolExecutorTests.classMethod
      1 org.opensearch.common.util.concurrent.QueueResizableOpenSearchThreadPoolExecutorTests.testResizeQueueDown

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

Copy link

codecov bot commented Apr 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.61%. Comparing base (b15cb0c) to head (15e994e).
Report is 228 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main   #13400      +/-   ##
============================================
+ Coverage     71.42%   71.61%   +0.19%     
- Complexity    59978    60863     +885     
============================================
  Files          4985     5045      +60     
  Lines        282275   286080    +3805     
  Branches      40946    41432     +486     
============================================
+ Hits         201603   204889    +3286     
- Misses        63999    64280     +281     
- Partials      16673    16911     +238     

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

@github-actions github-actions bot added bug Something isn't working Search:Aggregations labels Apr 26, 2024
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Great stuff.

You need to amend/sign your commits with DCO, -s, please?

@dblock
Copy link
Member

dblock commented Apr 26, 2024

Since these tests pass, make the changes in the comments as you suggested in #13375 and maybe open a documentation PR?

@Zhikai-VM Zhikai-VM force-pushed the bug/issue-13375-add-test-case-for-multi-term-order branch from f5a9520 to a3a1f23 Compare April 27, 2024 01:47
@Zhikai-VM Zhikai-VM force-pushed the bug/issue-13375-add-test-case-for-multi-term-order branch from a3a1f23 to 5257b82 Compare April 27, 2024 02:01
Copy link
Contributor

✅ Gradle check result for a3a1f23: SUCCESS

Copy link
Contributor

✅ Gradle check result for f5a9520: SUCCESS

Copy link
Contributor

✅ Gradle check result for 5257b82: SUCCESS

@Zhikai-VM
Copy link
Contributor Author

Since these tests pass, make the changes in the comments as you suggested in #13375 and maybe open a documentation PR?

I have updated the comments in 'MultiTermsAggregationBuilder.java'.

Copy link
Contributor

✅ Gradle check result for bd340cf: SUCCESS

@Zhikai-VM
Copy link
Contributor Author

Create a doc issue for this: opensearch-project/documentation-website#7037

@dblock
Copy link
Member

dblock commented Apr 27, 2024

Can you please rebase to get CI to pass? We fixed these errors in #13412, but need to make sure there's nothing else broken. Thanks!

1. test case for multiple orders
2. test case for multiple orders and size

Signed-off-by: Zhikai Chen <snoopy_czk@126.com>
Signed-off-by: Zhikai Chen <snoopy_czk@126.com>
@Zhikai-VM Zhikai-VM force-pushed the bug/issue-13375-add-test-case-for-multi-term-order branch from bd340cf to 15e994e Compare April 27, 2024 14:00
@Zhikai-VM
Copy link
Contributor Author

Can you please rebase to get CI to pass? We fixed these errors in #13412, but need to make sure there's nothing else broken. Thanks!

Done.

Copy link
Contributor

✅ Gradle check result for 15e994e: SUCCESS

@Zhikai-VM Zhikai-VM requested a review from dblock April 27, 2024 22:45
@Zhikai-VM
Copy link
Contributor Author

Great stuff.

You need to amend/sign your commits with DCO, -s, please?

Done.

@Zhikai-VM Zhikai-VM requested a review from ankyhe April 28, 2024 00:35
@Zhikai-VM
Copy link
Contributor Author

Hi @dblock , could you please kindly let me know what should I do next? (I have signed the commits, but still shows "1 change requested")

@dblock
Copy link
Member

dblock commented Apr 29, 2024

Hi @dblock , could you please kindly let me know what should I do next? (I have signed the commits, but still shows "1 change requested")

It's me! I needed to re-review and hit approve ;)

@dblock dblock added the backport 2.x Backport to 2.x branch label Apr 29, 2024
@dblock dblock merged commit 4ee984f into opensearch-project:main Apr 29, 2024
33 checks passed
@dblock
Copy link
Member

dblock commented Apr 29, 2024

Thanks for contributing @Zhikai-VM! Hope you can pickup something else ;)

opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 29, 2024
…3400)

* Add unit test cases for multiple terms aggregation:
1. test case for multiple orders
2. test case for multiple orders and size

Signed-off-by: Zhikai Chen <snoopy_czk@126.com>

* Update the comments.

Signed-off-by: Zhikai Chen <snoopy_czk@126.com>

---------

Signed-off-by: Zhikai Chen <snoopy_czk@126.com>
(cherry picked from commit 4ee984f)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
dblock pushed a commit that referenced this pull request Apr 29, 2024
…3400) (#13450)

* Add unit test cases for multiple terms aggregation:
1. test case for multiple orders
2. test case for multiple orders and size



* Update the comments.



---------


(cherry picked from commit 4ee984f)

Signed-off-by: Zhikai Chen <snoopy_czk@126.com>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@Zhikai-VM
Copy link
Contributor Author

Thanks for contributing @Zhikai-VM! Hope you can pickup something else ;)

Thanks for your help!

@ankyhe
Copy link

ankyhe commented May 6, 2024

@dblock We want to revise the official document also. Could you answer this comment: opensearch-project/documentation-website#7039 (comment)? Thanks.

@ankyhe
Copy link

ankyhe commented May 14, 2024

@dblock @bowenlan-amzn Could you have a check of this PR: opensearch-project/documentation-website#7039. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch bug Something isn't working Search:Aggregations skip-changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] The comments did not match the codes in MultiTermsAggregationBuilder
3 participants