Skip to content

Conversation

@mayya-sharipova
Copy link
Contributor

randomAlphaOfLength(2) produced duplicates which led to
com.fasterxml.jackson.core.JsonParseException: Duplicate field

increase it to randomAlphaOfLength(7)

closes #34837

randomAlphaOfLength(2) produced duplicates which led to
com.fasterxml.jackson.core.JsonParseException: Duplicate field

increase it to randomAlphaOfLength(7)

closes elastic#34837
}

TermVectorsResponse.TermVector tv = new TermVectorsResponse.TermVector("field" + randomAlphaOfLength(2), fs, terms);
TermVectorsResponse.TermVector tv = new TermVectorsResponse.TermVector("field" + randomAlphaOfLength(7), fs, terms);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be nice to check for duplicated before adding to the list and retry. Or to pass in a unique prefix to this method to be sure we don't have duplicates.

}

// generate a new string different from the supplied usedStrings
private String produceUniqueString(List<String> usedStrings) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think it woiuld make sense to use a Set here so you can simply user contains rather than having to iterate through the list. Not a big deal in this test case though since lists are very short, more as a general suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cbuescher Thanks for the suggestion, I will remember about this next time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a related note, there is a helper in ESTestCase that might be usable here: randomValueOtherThanMany(). I think in this method could be used here instead of produceUniqueString, somehow like String fieldName = randomValueOtherThanMany(t -> usedStrings.contains(t), () -> randomAlphaOfLength(7)) (I might have the predicate the wrong way around). Just as a suggestion as well.

@colings86
Copy link
Contributor

Please remember to add all relevant labels (area label, version label(s) and change type label) on all PRs and please look for this as part of reviews. The release note generation process is made much harder when PRs are not labelled correctly.

@colings86 colings86 added >test Issues or PRs that are addressing/adding tests :Search/Search Search-related issues that do not fall into other categories v7.0.0 labels Oct 26, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

@mayya-sharipova
Copy link
Contributor Author

@cbuescher Thanks for the suggestion, randomValueOtherThanMany is indeed a very useful function. I have modified the test to use it.

Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM

@mayya-sharipova mayya-sharipova merged commit fced5e8 into elastic:master Oct 26, 2018
@mayya-sharipova mayya-sharipova deleted the correct-TermVectorsResponseTests branch October 26, 2018 17:56
kcm pushed a commit that referenced this pull request Oct 30, 2018
* Increase the length of randomly generated field

randomAlphaOfLength(2) produced duplicates which led to
com.fasterxml.jackson.core.JsonParseException: Duplicate field

increase it to randomAlphaOfLength(7)

closes #34837
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Search/Search Search-related issues that do not fall into other categories >test Issues or PRs that are addressing/adding tests v7.0.0-beta1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Test] HLRC: TermVectorsResponseTests fails with some seeds

5 participants