Continue consolidating XContentParser construction in tests
#22145
Conversation
nik9000
added
review
>test
imotov
left a comment
imotov
left a comment
There was a problem hiding this comment.
LGTM in general, added a couple of optional suggestions.
There was a problem hiding this comment.
Maybe we should rename this class into AbstractYamlParserTestCase then?
There was a problem hiding this comment.
I am just wondering did you consider passing xContent type as a parameter here instead of overloading it on the suite level? I feel like it might provide more flexibility.
There was a problem hiding this comment.
I have a feeling I'm going to have a few more iterations of this as I do more centralizing. I'll stick with what I have for now and keep an open mind to that as I go. I'm not sure that we need the flexibility but the specificity might be nice....
There was a problem hiding this comment.
I have one objection, sorry @nik9000 ! We plan on removing content type auto-detection at some point (see #19388), which is going to be a big job. This PR replaces all the explicit parser creation code with auto-detection which will make it even harder to remove in the future. Why do we have to auto-detect when we exactly know the content type? I am tempted to deprecate those auto-detection methods cause we end up using them everywhere for no reason. I think we should rather pass around the known XContentType.
Oh boy! Ok. I'll go the other way.... |
We think we'll remove it from the system entirely eventually.
nik9000
force-pushed
the
test_parser_build_1
branch
from
e8201a3 to
917a6f9
Compare
|
@javanna, you caught me mid rebase. I did what you asked but it looks like I forgot to undo my rebase. So you get rebased commits. Can you have a look? |
|
I'm still running all the tests. I expect I'll find a few things and push fixes but you get the idea. |
|
looks good @nik9000 thanks a lot, that is what I meant. |
nik9000
changed the title
XContententParser construction in testsXContentParser construction in tests
Consolidate more parser creation in tests Moves more parser creation in tests to the `createParser` methods in `ESTestCase`.
3 participants
Continue consolidating
XContentParserconstruction in tests as a prerequisite to #22003