-
Notifications
You must be signed in to change notification settings - Fork 24.6k
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
Randomize Content-Type in REST tests #23245
Conversation
a5b829d
to
407457c
Compare
So... Why randomise it instead of always testing all content types? |
hi @KodrAus , this suite runs around 700 tests and takes a few minutes to run. The content type that is most important is json, the one that we've been using up until now. Running all the same tests with yaml, cbor and smile too would take too long, that is why randomizing helps. I think I will also go back and give a bit more importance to json though, given that it is the most widely used format. |
@javanna I thought that might be the case :) Thanks for taking the time to clarify! |
7904c42
to
fc2d893
Compare
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.
This is great; thanks for doing this @javanna. I left a few small comments but other than that LGTM
String key = entry.getKey(); | ||
String value = entry.getValue(); | ||
Map<String, Object> settings = new HashMap<>(); | ||
if (request.hasContent()) { |
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.
this is a nit IMO, but might be cleaner if the request#contentOrSourceParamParserOrNull
method is used?
binary: | ||
type: binary | ||
doc_values: true | ||
#binary: |
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.
should we remove all of these commented lines since it seems they have been moved to another test?
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.
leftover, will remove
if (bodies.size() == 1) { | ||
return bodyAsString(stash.replaceStashedValues(bodies.get(0))); | ||
XContentType xContentType = null; | ||
String contentType = headers.get("Content-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.
I think we may want to document that the headers used in the yaml tests are case sensitive or fix the map to be case insensitive?
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 was hoping you wouldn't catch this :)
fc2d893
to
830464f
Compare
c983838
to
832ef6b
Compare
0a8b356
to
d4d8cec
Compare
This allows to set content-type together with the body itself. At the moment it is always json, but this change allows makes it easier to randomize it later
…g tests This test makes little sense when sent from the REST layer, as WrapperQueryBuilder is supposed to be used from the Java api. Also, providing the inner query as base64 string will work only for string formats and break for binary formats like SMILE and CBOR, whcih doesn't play well with randomizing content type in our REST tests
…tent-Type header explicitly
Rather test that the size is present and greather than zero. The actual size depends on the content-type, which is randomized.
d4d8cec
to
0025724
Compare
This PR introduces randomization of the content type for requests sent as part of our REST tests. Requests can be sent in any supported format, and responses can be read from any of the supported formats too, according to the returned Content-Type header.
This new randomization revealed a couple of problems: