-
Notifications
You must be signed in to change notification settings - Fork 25k
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
TEST: avoid generating duplicate multiple fields #27080
Conversation
Multifields parser does not allow duplicate values, however the MultiFieldTests may produce duplicate field values. See https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+master+release-tests/132/console.
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, I left one comment.
@@ -155,8 +156,9 @@ public void testBuildThenParse() throws Exception { | |||
// can to unnecessary re-syncing of the mappings between the local instance and cluster state | |||
public void testMultiFieldsInConsistentOrder() throws Exception { | |||
String[] multiFieldNames = new String[randomIntBetween(2, 10)]; | |||
HashSet<String> seenFields = new HashSet<>(); |
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.
Let's type the left-hand side as Set<String>
.
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.
Ah, I prefer to have a concrete type for local variables. I will push the change.
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 the less-specific types are better when you can get away with them so that you do not come to rely on implementation details of the concrete type.
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.
Agreed!
Thanks @jasontedor, your comment is addressed. |
* master: Timed runnable should delegate to abstract runnable Expose adaptive replica selection stats in /_nodes/stats API Remove dangerous `ByteBufStreamInput` methods (elastic#27076) Blacklist Gradle 4.2 and above Remove duplicated test (elastic#27091) Update numbers to reflect 4-byte UTF-8-encoded characters (elastic#27083) test: avoid generating duplicate multiple fields (elastic#27080) Reduce the default number of cached queries. (elastic#26949) removed unused import ingest: date processor should not fail if timestamp is specified as json number
Multifields parser does not allow duplicate values, however the
MultiFieldTests
may produce duplicate field values.See https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+master+release-tests/132/console