-
Notifications
You must be signed in to change notification settings - Fork 84
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: got the client_integration_test moved over to the builder. #2362
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@goaway I expect this to fail CI as I haven't gotten the RTDS test passing yet, but client integration test is good to go and there's a lot of moving parts here I'd like your thoughts on, especially the weirdness of taking config from the builder, using it to initialize the integration test config helper, then serializing the config helper back to the builder so we can test custom configs. thanks! |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
library/cc/stream.h
Outdated
@@ -18,7 +18,7 @@ class Stream { | |||
Stream(envoy_engine_t engine_handle, envoy_stream_t handle); | |||
|
|||
Stream& sendHeaders(RequestHeadersSharedPtr headers, bool end_stream); | |||
Stream& sendData(envoy_data data); | |||
Stream& sendData(envoy_data data, bool end_stream); |
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.
IIRC the reason end_stream
is missing here was the "fluent"/chained interface design: one would call close(envoy data data)
to pass a final data frame, and that in turn would return void
instead of a reference to a now-closed stream, finishing off the chain.
I think we can reconsider whether that design actually makes sense here, but for now it's probably worth keeping the iOS/Android/C++ interfaces in alignment.
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.
but doesn't sendHeaders with end stream implicitly close? That's a pretty weird abstraction IMO but yeah this PR is not the place to change it.
Discussed a bit over Slack. Overall this seems fine to me, though at least on a case-by-case basis I think it's worth considering whether a given override might simply make sense as a Builder option. I'll also be thinking over possibilities for allowing for direct overrides - at least on C++ - rather than needing to do the serialization ahead of time and passing that back through. |
ok as discussed, moved the default to "use builder config" but I don't think we want to block this PR on builder RTDS support. |
@@ -46,6 +47,9 @@ class EngineBuilder { | |||
// Filter): EngineBuilder& addNativeFilter(name: String = UUID.randomUUID().toString(), | |||
// typedConfig: String): EngineBuilder& addStringAccessor(name: String, accessor: | |||
// EnvoyStringAccessor): EngineBuilder { | |||
protected: | |||
void setOverrideConfigForTests(std::string config) { config_override_for_tests_ = config; } | |||
void setAdminAddressPathForTests(std::string admin) { admin_address_path_for_tests_ = admin; } |
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.
alternately could publicly add key-value and hash map for generic command line override.
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@@ -28,6 +28,11 @@ RequestHeadersBuilder::addUpstreamHttpProtocol(UpstreamHttpProtocol upstream_htt | |||
return *this; | |||
} | |||
|
|||
RequestHeadersBuilder& RequestHeadersBuilder::addKeyValue(std::string key, std::string value) { |
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.
nit: I'd call thisset
, for alignment with the other builders: https://github.com/envoyproxy/envoy-mobile/blob/main/library/kotlin/io/envoyproxy/envoymobile/RequestHeadersBuilder.kt#L43
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.
One minor nit, but otherwise LGTM. Thanks @alyssawilk!
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
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.
Yay, one more up to date API!
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
followup to #2362 Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Risk Level: low Testing: yep Release Notes: inline Fixes #2303 Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
followup to #2362 Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
In the commit #2362 we added a custom_headers_ field on the BaseClientIntegrationTest class to use for custom headers. This commit replaces that class field by instead instantiating a custom_headers object inside tests when necessary. Then adds a helper function to convert it from the TestRequestHeaderMapImpl (an easy to edit type) to RequestHeaders (envoy header type). Signed-off-by: caschoener schoener@google.com
Risk Level: low
Testing: yep
Release Notes: inline
Fixes #2303