-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Added unit test coverage for SignificantTerms #24904
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add some of the tests from the removed integration test suite. Specifically I think we should test with include/exclude, with a different heuristic and unmapped/partially unmapped fields
@markharwood Do you want to address Colin's feedback and get this in? |
Looking at this now |
361a85f
to
f16ad5a
Compare
@colings86 Care to take another look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left one comment but LGTM
|
||
// Search "odd" | ||
SignificantTerms terms = searchAndReduce(searcher, new TermQuery(new Term("text", "odd")), sigAgg, textFieldType); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we check that the number of buckets returned is zero instead to ensure nothing weird is going on?
f16ad5a
to
b504815
Compare
…, GlobalOrdinalsSignificantTermsAggregator.WithHash, SignificantLongTermsAggregator and SignificantStringTermsAggregator Removed integration test Relates elastic#22278
…ound_filter. Requested include/exclude tests already exist in this unit test.
…us check on number of segments which was recently causing issues in SignificantTextAggregatorTests
b504815
to
402f62a
Compare
jenkins test this |
* master: Move more token filters to analysis-common module Test: Allow merging mock secure settings (elastic#25387) Remove remaining `index.mapper.single_type` setting usage from tests (elastic#25388) Remove dead logger prefix code Tests: Improve stability and logging of TemplateUpgradeServiceIT tests (elastic#25386) Remove `index.mapping.single_type=false` from reindex tests (elastic#25365) Adapt `SearchIT#testSearchWithParentJoin` to new join field (elastic#25379) Added unit test coverage for SignificantTerms (elastic#24904)
Covers GlobalOrdinalsSignificantTermsAggregator, GlobalOrdinalsSignificantTermsAggregator.WithHash, SignificantLongTermsAggregator and SignificantStringTermsAggregator
Removed integration test
Relates #22278