-
Notifications
You must be signed in to change notification settings - Fork 668
DYN-6094 Lucene unit testing #14214
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
DYN-6094 Lucene unit testing #14214
Conversation
RobertGlobant20
left a comment
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.
LGTM with some comments
| //} | ||
|
|
||
| var ele = AddNodeTypeToSearch(cbnData); | ||
| var ele = AddNodeTypeToSearch(outputData); |
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.
just consider (for future bug fixes) that uncommenting this will probably generate duplicates in output and input nodes when searching in the Library (due that are already added in library).
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.
Thanks @RobertGlobant20. Will test this out.
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.
LGTM
The regressions reported is because this branch is not up to date with master latest.
just curious did we have to update the result of any previous search tests?
RobertGlobant20
left a comment
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.
LGTM with one comment
src/DynamoCoreWpf/ViewModels/Search/NodeAutoCompleteSearchViewModel.cs
Outdated
Show resolved
Hide resolved
|
@QilongTang updated to latest master now. did not have to change any search results for now. |
|
hi @reddyashish Let us know when this is ready for one more review |
|
|
||
|
|
||
| // Create an analyzer to process the text | ||
| Analyzer = CreateAnalyzerByLanguage(dynamoModel.PreferenceSettings.Locale); |
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.
Added back the lucene search support for different languages.
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.
@reddyashish you miss to comment/remove the line 86 otherwise we will be using always the StandardAnalyzer.

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.
Addressing it in here: #14248
| if (useLucene) | ||
| { | ||
| //The DirectoryReader and IndexSearcher have to be assigned after commiting indexing changes and before executing the Searcher.Search() method, otherwise new indexed info won't be reflected | ||
| LuceneSearchUtility.dirReader = LuceneSearchUtility.writer?.GetReader(applyAllDeletes: true); |
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.
All the search code is moved to the NodeSearchModel class.
|
@QilongTang This is now ready for final review
|
|
@reddyashish Did you run self serve on this branch or confirm the testing state? If yes, good to merge |
|
Yes, I ran the self-serve and only 5 docbrowser tests are failing. |
|
Merging this and will cherrypick into 2.19 |
* Lucene unit testing * test updates * Update LuceneSearchUtility.cs * Update NodeAutoCompleteSearchViewModel.cs * Update NodeAutoCompleteSearchViewModel.cs * Test updates (cherry picked from commit 780f854)
* DYN-6094 Lucene unit testing (#14214) * Lucene unit testing * test updates * Update LuceneSearchUtility.cs * Update NodeAutoCompleteSearchViewModel.cs * Update NodeAutoCompleteSearchViewModel.cs * Test updates (cherry picked from commit 780f854) * Update SearchResultDataProvider.cs (#14253) Co-authored-by: pinzart <tiberiu.pinzariu@autodesk.com> (cherry picked from commit aaf6a4e) --------- Co-authored-by: pinzart90 <46732933+pinzart90@users.noreply.github.com>


Purpose
This PR is to use Lucene search in the Dynamo unit tests.
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
DYN-6094 Lucene unit testing
Reviewers
@QilongTang @RobertGlobant20
FYIs
@DynamoDS/dynamo