-
Notifications
You must be signed in to change notification settings - Fork 243
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
HSEARCH-3467 Add tests for all the attributes validated by the Elasticsearch schema validator #1953
HSEARCH-3467 Add tests for all the attributes validated by the Elasticsearch schema validator #1953
Conversation
Pull Request Test Coverage Report for Build 7
💛 - Coveralls |
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.
Looks good so far. I don't really understand why you changed the structure of the tests, but I guess you have your reasons.
Be careful about validation of analysis definitions, though: some (all?) of it is already addressed in separate tests:
org.hibernate.search.integrationtest.backend.elasticsearch.management.ElasticsearchNormalizerDefinitionValidationIT
org.hibernate.search.integrationtest.backend.elasticsearch.management.ElasticsearchAnalyzerDefinitionValidationIT
...search/integrationtest/backend/elasticsearch/management/ElasticsearchSchemaValidationIT.java
Outdated
Show resolved
Hide resolved
...tegrationtest/backend/elasticsearch/management/ElasticsearchSchemaAttributeValidationIT.java
Show resolved
Hide resolved
528812e
to
b02927d
Compare
@yrodiere thank you for the review. |
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 added a few comments; most of them are just nitpicking so feel free to ignore if you disagree, but the ones about fielddata and docvalues are, I think, important.
...tegrationtest/backend/elasticsearch/management/ElasticsearchSchemaAttributeValidationIT.java
Show resolved
Hide resolved
...tegrationtest/backend/elasticsearch/management/ElasticsearchSchemaAttributeValidationIT.java
Show resolved
Hide resolved
...tegrationtest/backend/elasticsearch/management/ElasticsearchSchemaAttributeValidationIT.java
Outdated
Show resolved
Hide resolved
...tegrationtest/backend/elasticsearch/management/ElasticsearchSchemaAttributeValidationIT.java
Outdated
Show resolved
Hide resolved
...tegrationtest/backend/elasticsearch/management/ElasticsearchSchemaAttributeValidationIT.java
Outdated
Show resolved
Hide resolved
...tegrationtest/backend/elasticsearch/management/ElasticsearchSchemaAttributeValidationIT.java
Outdated
Show resolved
Hide resolved
...tegrationtest/backend/elasticsearch/management/ElasticsearchSchemaAttributeValidationIT.java
Outdated
Show resolved
Hide resolved
Moving all *attribute* tests from ElasticsearchSchemaValidationIT to ElasticsearchSchemaAttributeValidationIT class
b02927d
to
504d785
Compare
Thanks @yrodiere. I think I've addressed the points here. |
Merged, thanks! |
Thanks @yrodiere. I'm reviewing yours. |
https://hibernate.atlassian.net/browse/HSEARCH-3467
Still in preview, since several attributes have not been tested yet:donebootsremoved!,store, null_value, fielddata, doc_values, normalizerdone!.Normalizer definition attributes: char_filter, token_filter.already existsAnalyzer definition attributes: char_filter, token_filter, tokenizer.already exists