test: deflaking a test, improving debugability by alyssawilk · Pull Request #3829 · envoyproxy/envoy

alyssawilk · 2018-07-10T15:56:53Z

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

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

htuch

Thanks, this is really helpful.

htuch · 2018-07-10T17:07:48Z

source/common/common/assert.h

+/**
+ * A version of RELEASE_ASSERT that allows for ostream style extra data.
+ */
+#define VERBOSE_RELEASE_ASSERT(X, Y)                                                               \


Should this just be the RELEASE_ASSERT implementation in the interest of economy of mechanism?

FWIW, I've wanted something like this for a while, so I would be in favor of adding this.

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 :-)

Any reason not to use the existing fmtlib-style for this?

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.

I prefer consistency with the code base, but I don't feel strongly about it. Anyone else have an opinion on this?

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.

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...

htuch · 2018-07-10T17:08:04Z

source/common/common/assert.h

+  do {                                                                                             \
+    if (!(X)) {                                                                                    \
+      std::ostringstream stream;                                                                   \
+      stream << Y;                                                                                 \


Should this be (Y) for the usual preprocessor reasons?

htuch · 2018-07-10T17:13:17Z

source/common/common/assert.h

+/**
+ * A version of RELEASE_ASSERT that allows for ostream style extra data.
+ */
+#define VERBOSE_RELEASE_ASSERT(X, Y)                                                               \


I think we want something closer to what happens in gtests, e.g. ASSERT_EQ(foo, bar) << baz;

htuch · 2018-07-10T17:13:58Z

test/integration/http2_integration_test.cc

  fake_upstream_connection1->waitForDisconnect();
  fake_upstream_connection2->close();
  fake_upstream_connection2->waitForDisconnect();
+  codec_client_->close();


Can you add a comment explaining how the ordering here matters/helps?

This one doesn't actually matter. I added it in case future code copy-pasted and it did matter.

htuch · 2018-07-10T17:14:37Z

test/integration/http_integration.cc

+  // 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.


Like this comment ;)

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

htuch

Ta muchly.

* 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>

test: deflaking a test, improving debugability

02639b5

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

htuch suggested changes Jul 10, 2018

View reviewed changes

htuch self-assigned this Jul 10, 2018

alyssawilk added 2 commits July 10, 2018 14:17

unit test and fix_format

29ce8f1

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Backing logging changes out, and putting them in their own PR

fe102b7

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

htuch approved these changes Jul 10, 2018

View reviewed changes

mattklein123 merged commit ce83509 into envoyproxy:master Jul 10, 2018

alyssawilk mentioned this pull request Jul 11, 2018

logging: Requiring details for RELEASE_ASSERT #3842

Merged

Conversation

alyssawilk commented Jul 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk Jul 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alyssawilk commented Jul 10, 2018 •

edited

Loading

alyssawilk Jul 10, 2018 •

edited

Loading