test: deflaking a test, improving debugability #3829
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
source/common/common/assert.h
Outdated
| /** | ||
| * A version of RELEASE_ASSERT that allows for ostream style extra data. | ||
| */ | ||
| #define VERBOSE_RELEASE_ASSERT(X, Y) \ |
There was a problem hiding this comment.
Should this just be the RELEASE_ASSERT implementation in the interest of economy of mechanism?
There was a problem hiding this comment.
FWIW, I've wanted something like this for a while, so I would be in favor of adding this.
There was a problem hiding this comment.
Cool. If you're up for it, I'm all for doing the more invasive ostream style default-release-assert. And possibly moving the other asserts over as well.
I think at that point I'll probably split it out of the test flake PR though :-)
There was a problem hiding this comment.
Any reason not to use the existing fmtlib-style for this?
There was a problem hiding this comment.
My weak argument is to be consistent with the ASSERT_EQ/EXPECT_EQ code in test but the real reason is I like ostream way better. I believe that fmt is more consistent with the code base so alternately I could look into a variable [1 or 2] argument macro with a fmt string as the second argument.
There was a problem hiding this comment.
I prefer consistency with the code base, but I don't feel strongly about it. Anyone else have an opinion on this?
There was a problem hiding this comment.
An argument in favor of ostream is that if we implement it that way, we can easily add the fmtlib variant as a wrapper on top, e.g.
#define RELEASE_ASSERT(X, Y, ...) RELEASE_ASSERT(X) << fmt::format(Y, __VARARGSTUFF_)
so, doing it ostream provides maximum flexibility.
There was a problem hiding this comment.
No strong opinion from me. Whatever you all think is best. Happy to see this land though, there are many times in prod code when I think it would be nice to actually have a more detailed message of a RELEASE_ASSERT() (I would go as far as to say we should ultimately audit all uses and force everyone to use the new version). Agree this can be split out though...
source/common/common/assert.h
Outdated
| do { \ | ||
| if (!(X)) { \ | ||
| std::ostringstream stream; \ | ||
| stream << Y; \ |
There was a problem hiding this comment.
Should this be (Y) for the usual preprocessor reasons?
source/common/common/assert.h
Outdated
| /** | ||
| * A version of RELEASE_ASSERT that allows for ostream style extra data. | ||
| */ | ||
| #define VERBOSE_RELEASE_ASSERT(X, Y) \ |
There was a problem hiding this comment.
I think we want something closer to what happens in gtests, e.g. ASSERT_EQ(foo, bar) << baz;
| fake_upstream_connection1->close(); | ||
| fake_upstream_connection1->waitForDisconnect(); | ||
| fake_upstream_connection2->close(); | ||
| fake_upstream_connection2->waitForDisconnect(); | ||
| codec_client_->close(); |
There was a problem hiding this comment.
Can you add a comment explaining how the ordering here matters/helps?
There was a problem hiding this comment.
This one doesn't actually matter. I added it in case future code copy-pasted and it did matter.
test/integration/http_integration.cc
Outdated
| // Close the upstream connection first. If there's an outstanding request, | ||
| // closing the client may result in a FIN being sent upstream, and FakeConnectionBase::close | ||
| // will interpret that as an unexpected disconnect. The codec client is not | ||
| // subject to the same failure mode. |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
* origin/master: config: making v2-config-only a boolean flag (envoyproxy#3847) lc trie: add exclusive flag. (envoyproxy#3825) upstream: introduce PriorityStateManager, refactor EDS (envoyproxy#3783) test: deflaking header_integration_test (envoyproxy#3849) http: new style WebSockets, where headers and data are processed by the filter chain. (envoyproxy#3776) common: minor doc updates (envoyproxy#3845) fix master build (envoyproxy#3844) logging: Requiring details for RELEASE_ASSERT (envoyproxy#3842) test: add test for consistency of RawStatData internal memory representation (envoyproxy#3843) common: jittered backoff implementation (envoyproxy#3791) format: run buildifier on .bzl files. (envoyproxy#3824) Support mutable metadata for endpoints (envoyproxy#3814) test: deflaking a test, improving debugability (envoyproxy#3829) Update ApiConfigSource docs with grpc_services only for GRPC configs (envoyproxy#3834) Add hard-coded /hot_restart_version test (envoyproxy#3832) healthchecks: Add interval_jitter_percent healthcheck option (envoyproxy#3816) Signed-off-by: Snow Pettersen <snowp@squareup.com>
Yet another flaky test fixed, with improvements to our test framework to make it less likely to reoccur, and a few minor logging additions to improve debuggability.
The failure mode here was TestBind sending a partial request, then closing the client connection (which sent a FIN upstream). 1:1000 runs that FIN would arrive during the fakeUpstream close() so before waitForDisconnect, which would cause an assert failure.
Once the behavior under test is complete, we should be able to clean up without crashes, so changing cleanupUpstreamAndDownstream to be resilient to this and using it. I don't think we want to make close always accept disconnects since that could result in overlooking unexpected behavior.
Risk Level: Low (test only)
Testing: TestBind now passes 10k runs.
Docs Changes: n/a
Release Notes: n/a