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
Remove UpdateSettingsTestHelper class #32557
Remove UpdateSettingsTestHelper class #32557
Conversation
By making the `settings()` method public on `UpdateSettingsRequest` (I think it should have been in the first place) we can get rid of this class entirely. Mock response objects are now constructed by parsing JSON without making the constructor public. Relates to elastic#29823
Pinging @elastic/es-core-infra |
final UpdateSettingsResponse response; | ||
try (XContentParser parser = XContentType.JSON.xContent() | ||
.createParser(NamedXContentRegistry.EMPTY, THROW_UNSUPPORTED_OPERATION, "{\"acknowledged\": true}")) { | ||
response = UpdateSettingsResponse.fromXContent(parser); |
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.
😄
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 ended up using the UpdateSettingsAction.INSTANCE.newResponse()
then response.readFrom(StreamInput.wrap(new byte[] { ... }))
route for this workaround. But I like this better! more self-describing
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.
Can we not make the Response constructor public here? Having the object parsed from a JSON string seems fragile and ugly to me.
…-update-settings-test-helper
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'm not sure about having the response parsed from a JSON string, it feels ugly to me and IMO more ugly than having the UpdateSettingsTestHelper class. I'm wondering if we can't just make the Response constructor public?
…-update-settings-test-helper
…-update-settings-test-helper
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
…-update-settings-test-helper
…-update-settings-test-helper
…-update-settings-test-helper
* Remove UpdateSettingsTestHelper class By making the `settings()` method public on `UpdateSettingsRequest` (I think it should have been in the first place) we can get rid of this class entirely. Mock response objects are now constructed by parsing JSON without making the constructor public. Relates to #29823
By making the
settings()
method public onUpdateSettingsRequest
(I think itshould have been in the first place) we can get rid of this class entirely. Mock
response objects are now constructed by parsing JSON without making the
constructor public.
Relates to #29823