Skip to content

Conversation

@reddyashish
Copy link
Contributor

@reddyashish reddyashish commented Aug 1, 2023

Purpose

This PR is to use Lucene search in the Dynamo unit tests.

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated
  • This PR contains no files larger than 50 MB

Release Notes

DYN-6094 Lucene unit testing

Reviewers

@QilongTang @RobertGlobant20

FYIs

@DynamoDS/dynamo

Copy link
Contributor

@RobertGlobant20 RobertGlobant20 left a 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);
Copy link
Contributor

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).

Copy link
Contributor Author

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.

@avidit avidit changed the title Lucene unit testing DYN-6094 Lucene unit testing Aug 2, 2023
Copy link
Contributor

@QilongTang QilongTang left a 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?

Copy link
Contributor

@RobertGlobant20 RobertGlobant20 left a 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

@reddyashish
Copy link
Contributor Author

@QilongTang updated to latest master now. did not have to change any search results for now.

@QilongTang
Copy link
Contributor

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);
Copy link
Contributor Author

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.

Copy link
Contributor

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.
image

Copy link
Contributor Author

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);
Copy link
Contributor Author

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.

@reddyashish
Copy link
Contributor Author

@QilongTang This is now ready for final review
Main changes in recent commit:

  1. Updated SearchModelTests to use the lucene search model.
  2. Moved the lucene search code from ViewModel to Model.
  3. Added back the lucene search support for different languages.

@reddyashish
Copy link
Contributor Author

reddyashish commented Aug 10, 2023

The failing tests here are from documentationbrowser and not related to this PR.

EngOps build has failed with this error, which is weird as those DLL's are present.
Screenshot 2023-08-10 at 6 40 46 AM

@QilongTang
Copy link
Contributor

The failing tests here are from documentationbrowser and not related to this PR.

EngOps build has failed with this error, which is weird as those DLL's are present. Screenshot 2023-08-10 at 6 40 46 AM

Those binaries are not under AnyCPU fiolder for now

@QilongTang QilongTang added this to the 2.19.0 milestone Aug 10, 2023
@QilongTang
Copy link
Contributor

QilongTang commented Aug 10, 2023

@reddyashish Did you run self serve on this branch or confirm the testing state? If yes, good to merge

@reddyashish
Copy link
Contributor Author

Yes, I ran the self-serve and only 5 docbrowser tests are failing.

@reddyashish
Copy link
Contributor Author

Merging this and will cherrypick into 2.19

@reddyashish reddyashish merged commit 780f854 into DynamoDS:master Aug 10, 2023
reddyashish added a commit to reddyashish/Dynamo that referenced this pull request Aug 14, 2023
* Lucene unit testing

* test updates

* Update LuceneSearchUtility.cs

* Update NodeAutoCompleteSearchViewModel.cs

* Update NodeAutoCompleteSearchViewModel.cs

* Test updates

(cherry picked from commit 780f854)
QilongTang pushed a commit that referenced this pull request Aug 15, 2023
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants