test: Adds an integration test for SDS#2395
Conversation
As part of this change, common xDS test functionality has been factored out into the XdsIntegrationTest class, and both the SdsIntegrationTest and RtdsIntegrationTest subclass XdsIntegrationTest. Future xDS integration tests should also subclass the XdsIntegrationTest to get the common functionality and setup. Signed-off-by: Ali Beyad <abeyad@google.com>
|
cc @alyssawilk |
Signed-off-by: Ali Beyad <abeyad@google.com>
Signed-off-by: Ali Beyad <abeyad@google.com>
Signed-off-by: Ali Beyad <abeyad@google.com>
Signed-off-by: Ali Beyad <abeyad@google.com>
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
Looking good! Sorry I missed a bunch of things yesterday, but I think this is about it for alyssa-nits!
Signed-off-by: Ali Beyad <abeyad@google.com>
|
I pushed a commit addressing the feedback, PTAL! |
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
ah I think other tests just pass the getParam into the base class but I'm fine with improved readability. Just the one nit and you're good to go!
RyanTheOptimist
left a comment
RyanTheOptimist
left a comment
There was a problem hiding this comment.
sigh I meant to send these yesterday but forgot to click send. sigh
| using ::testing::AssertionResult; | ||
| using ::testing::AssertionSuccess; | ||
|
|
||
| XdsIntegrationTest::XdsIntegrationTest() : BaseClientIntegrationTest(ipVersion()) { |
There was a problem hiding this comment.
Is there an easy way to see what in this file is new, and what is a copy from test/common/integration/rtds_integration_test.cc?
There was a problem hiding this comment.
Unfortunately I don't think so :(
I should've done a git cp instead of creating the file from scratch.
|
|
||
| void addRuntimeRtdsConfig() { | ||
| // Add the layered runtime config, which includes the RTDS layer. | ||
| RtdsIntegrationTest() : XdsIntegrationTest() { |
There was a problem hiding this comment.
nit: Is the ": XdsIntegrationTest()" required, or does C++ magically do the right thing without it?
There was a problem hiding this comment.
C++ does the right thing since it's a zero arg constructor so it automatically calls the parent class' zero-arg constructor. I removed the explicit call.
Signed-off-by: Ali Beyad <abeyad@google.com>
abeyad
left a comment
abeyad
left a comment
There was a problem hiding this comment.
Pushed a commit addressing the latest round of feedback.
|
|
||
| void addRuntimeRtdsConfig() { | ||
| // Add the layered runtime config, which includes the RTDS layer. | ||
| RtdsIntegrationTest() : XdsIntegrationTest() { |
There was a problem hiding this comment.
C++ does the right thing since it's a zero arg constructor so it automatically calls the parent class' zero-arg constructor. I removed the explicit call.
| using ::testing::AssertionResult; | ||
| using ::testing::AssertionSuccess; | ||
|
|
||
| XdsIntegrationTest::XdsIntegrationTest() : BaseClientIntegrationTest(ipVersion()) { |
There was a problem hiding this comment.
Unfortunately I don't think so :(
I should've done a git cp instead of creating the file from scratch.
RyanTheOptimist
left a comment
RyanTheOptimist
left a comment
There was a problem hiding this comment.
LGTM (though it's annoying that github doesn't help make it easy to see some of changes. sigh :>)
* origin/main: Update Envoy (#2403) connectivity_manager: rename/refactor from Network::Configurator (#2401) test: fixing up build targets to use the extension registry (#2397) test: Adds an integration test for SDS (#2395) iOS: add `forceIPv6(...)` builder option (#2396) api: make iOS Headers and HeadersBuilder case-insensitive (#2383) Signed-off-by: JP Simard <jp@jpsim.com>
3 participants
As part of this change, common xDS test functionality has been factored
out into the XdsIntegrationTest class, and both the SdsIntegrationTest
and RtdsIntegrationTest subclass XdsIntegrationTest. Future xDS
integration tests should also subclass the XdsIntegrationTest to get the
common functionality and setup.
Signed-off-by: Ali Beyad abeyad@google.com