Moving more tests over to the v2 API#2204
Conversation
Signed-off-by: Alyssa Wilk <[email protected]>
| EXPECT_STREQ("200", response->headers().Status()->value().c_str()); | ||
| EXPECT_THAT(response->body(), testing::HasSubstr("added_via_api")); | ||
| EXPECT_THAT(response->body(), testing::HasSubstr("version_info::\n")); | ||
| EXPECT_THAT(response->body(), testing::HasSubstr("version_info::static\n")); |
There was a problem hiding this comment.
This is the one functional change I'm unsure about. Hadn't tracked down the cause.
There was a problem hiding this comment.
I'm guessing it's because server.json has CDS setup but there is no version info yet. This raises a point that for better or worse server.json has a bunch of random stuff in it mainly just to make sure that basic config elements don't break even if there is no usable upstream CDS server, stats backend, etc. We should try to make sure that we don't lose this "coverage."
There was a problem hiding this comment.
Sorry, I'm not sure I'm understanding the ask here.
Are you asking that the utility class preserve standard things like health checking + admin config from server.json rather than the relevant tests adding them, or something else?
There was a problem hiding this comment.
I'm not really asking for anything. I was just making a comment that there is some test coverage that we had gained from the server.json file that we may lose with the simplistic basic config template and targeted additions we are now using for v2 tests (as seen by this delta). It would be worth an audit of the delta to see if we are losing anything and want to bring any of it back in a targeted "sanity test" of random features. We can do this in follow ups...
There was a problem hiding this comment.
Ahhh, so we're missing coverage of config loading if those features aren't explicitly tested. Sure I'll take a look at what's missing once I get the rest of the tests moved over.
| TestHeaderMapImpl(const std::initializer_list<std::pair<std::string, std::string>>& values); | ||
| TestHeaderMapImpl(const HeaderMap& rhs); | ||
|
|
||
| friend std::ostream& operator<<(std::ostream& os, const TestHeaderMapImpl& p) { |
There was a problem hiding this comment.
Wherever you found this useful, I think if you included printers.h it would "just work" without this change. I've never found a great way of dealing with the existence of printers.h. I almost would prefer to auto include it in all tests. Worth exploring?
There was a problem hiding this comment.
Oh interesting. I was wondering what was up with printers.h
Unfortunately that fix doesn't seem to work for my use case. Am I doing something wrong?
Removing my ostream and adding
#include "test/test_common/printers.h"
to the top of cors_filter_integration_test.cc and tweaking the test to fail I get the ugly version again
test/integration/cors_filter_integration_test.cc:90: Failure
Expected equality of these values:
expected_response_headers
Which is: 512-byte object <80-3A AC-01 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 D0-43 83-04 00-00 00-00 10-4A 83-04 00-00 00-00 10-40 83-04 00-00 00-00 00-00 00-00 00-00 00-00 90-42 83-04 00-00 00-00 ... 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 C0-43 83-04 00-00 00-00 C0-48 83-04 00-00 00-00>
response_headers
Which is: 512-byte object <80-3A AC-01 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 10-AF 72-04 00-00 00-00 50-CB 77-04 00-00 00-00 10-CA 77-04 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 ... 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-AF 72-04 00-00 00-00 80-D6 77-04 00-00 00-00>
There was a problem hiding this comment.
Hmm. TBH I have no idea off the top of my head. I would think that printers.h void PrintTo(const HeaderMap& headers, std::ostream* os); would cover this case, but maybe it's not selecting the base class version appropriately? This is not a big deal. I actually think we should get rid of printers.h/printers.cc and just put the printers next to the class definitions, even if they are in public code (and just add some basic tests). WDYT? Open up an issue to track that?
There was a problem hiding this comment.
Please add any details you think would be helpful!
| EXPECT_STREQ("200", response->headers().Status()->value().c_str()); | ||
| EXPECT_THAT(response->body(), testing::HasSubstr("added_via_api")); | ||
| EXPECT_THAT(response->body(), testing::HasSubstr("version_info::\n")); | ||
| EXPECT_THAT(response->body(), testing::HasSubstr("version_info::static\n")); |
There was a problem hiding this comment.
I'm guessing it's because server.json has CDS setup but there is no version info yet. This raises a point that for better or worse server.json has a bunch of random stuff in it mainly just to make sure that basic config elements don't break even if there is no usable upstream CDS server, stats backend, etc. We should try to make sure that we don't lose this "coverage."
|
@alyssawilk thanks! This is helpful re: #2201. |
Signed-off-by: JP Simard <[email protected]>
Signed-off-by: JP Simard <[email protected]>
3 participants
Risk Level: Low (test only)
Release Notes: (n/a)