test: Add timeouts to methods that could wait forever in test/integration/fake_upstream.h. by mkbehr · Pull Request #3936 · envoyproxy/envoy

mkbehr · 2018-07-23T23:07:38Z

Description: Part of work for #3809. This changes methods that wait for something to happen, in FakeUpstream and related classes, so that they take an optional timeout arg. Timeouts default to 10 seconds. The methods now all return AssertionResults, marked with ABSL_MUST_USE_RESULT to require tests to check for failure. Methods that used to return something else now use an output argument.
Risk Level: Low (test-only, will break builds)
Testing: bazel test //test/...
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: Michael Behr <mkbehr@google.com>

lizan · 2018-07-23T23:25:19Z

test/common/grpc/grpc_client_integration_test.cc

 public:
  void expectExtraHeaders(FakeStream& fake_stream) override {
-    fake_stream.waitForHeadersComplete();
+    ASSERT(fake_stream.waitForHeadersComplete());


This will be compiled out in release builds, which is not your intention, no?

Right, yeah. Changed these to use RELEASE_ASSERT. Do you think it's worth adding a macro for this use? (RELEASE_ASSERT on an AssertionResult, using the AssertionResult's message as the RELEASE_ASSERT's message)

Why not use ASSERT_TRUE?

ASSERT_TRUE will return from the helper function but keep the test running, so the test will probably crash anyway but in a more confusing way. (And for helpers that return things, like SquashFilterIntegrationTest::sendSquash, it won't compile.)

…rors. Signed-off-by: Michael Behr <mkbehr@google.com>

…eout Signed-off-by: Michael Behr <mkbehr@google.com>

mkbehr · 2018-07-24T20:14:03Z

Having some trouble figuring out why tests are passing on my machine but failing on circleci.

zuercher · 2018-07-24T22:29:16Z

The error for all those builds is:

external/envoy/test/integration/autonomous_upstream.cc:76:3: error: ignoring return value of function declared with 'warn_unused_result' attribute [-Werror,-Wunused-result]
  http_connection->initialize();
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

On line 345 of fake_upstream.h you add ABSL_MUST_USE_RESULT to the FakeConnectionBase::initialize() method (which returns a testing::AssertionResult). Because of the annotation, you must now do something with that AssertionResult at every call site.

If it doesn't fail locally, perhaps your compiler doesn't support the annotation?

RELEASE_ASSERT in AutonomousUpstream::createNetworkFilterChain; ABSL_MUST_USE_RESULT on FakeRawConnection::initialize Signed-off-by: Michael Behr <mkbehr@google.com>

…eout Signed-off-by: Michael Behr <mkbehr@google.com>

mkbehr · 2018-07-25T16:58:01Z

It's strange that the release build passes even though it ignores that value from autonomous_upstream.cc. It also builds fine on my machine, even though my compiler will complain about dropping results from ABSL_MUST_USE_RESULT in other places. But since the CI caught it I won't beat my head against this too long.

zuercher · 2018-07-25T18:06:03Z

There's at least one more instance of that problem, unfortunately. The ASAN, TSAN, Mac and (unexpectedly to me) IPv6 builds all use clang. Release and coverage use gcc. So that's why it only happens in those builds.

mkbehr · 2018-07-25T19:49:46Z

I'll get the ci builds running on my machine so I can test with them directly.

Signed-off-by: Michael Behr <mkbehr@google.com>

…eout Signed-off-by: Michael Behr <mkbehr@google.com>

…e-upstream-timeout Signed-off-by: Michael Behr <mkbehr@google.com>

zuercher

Looks good! Some minor changes I'd like to see.

zuercher · 2018-07-30T17:05:50Z

test/integration/fake_upstream.h

-  void waitForReset();
+  ABSL_MUST_USE_RESULT
+  testing::AssertionResult
+  waitForHeadersComplete(std::chrono::milliseconds timeout = std::chrono::milliseconds(10000));


Consider replacing std::chrono::milliseconds(10000) with a static constant.

Also I think it would be easier to read these 3-line declarations if they had a blank line between them.

Done (and the rest of these too).

zuercher · 2018-07-30T17:19:05Z

test/test_common/utility.h

  } while (false)

+#define VERIFY_ASSERTION(statement)                                                                \
+  {                                                                                                \


do { ... } while(false) as in the other macros. (This will force VERIFY_ASSERTION() to be followed by a semicolon in all cases, which I think we want.)

zuercher · 2018-07-30T17:22:51Z

test/integration/fake_upstream.h

+  // wishes to wait for a new stream, set ignore_spurious_events = true.
+  ABSL_MUST_USE_RESULT
+  testing::AssertionResult
+  waitForNewStream(Event::Dispatcher& client_dispatcher, FakeStreamPtr* stream,


Our style guide prefers using a reference for connection here, since it can't be nullptr. Also add a comment indicating that the connection is returned via the reference. (Likewise for waitForRawConnection and waitForHttpConnection.)

…e-upstream-timeout Signed-off-by: Michael Behr <mkbehr@google.com>

- Static constant for default timeouts - do while false for VERIFY_ASSERTION - Reference for output arguments in waitForNewStream et al. Signed-off-by: Michael Behr <mkbehr@google.com>

…e-upstream-timeout Signed-off-by: Michael Behr <mkbehr@google.com>

zuercher

Looks good. Thanks for taking this on.

mattklein123

Super cool, thank you!

mkbehr added 2 commits July 23, 2018 17:53

Add timeouts to methods in fake_upstream.h that could wait forever.

1ee408c

Signed-off-by: Michael Behr <mkbehr@google.com>

Fix tests outside of test/integration.

e4dcd85

Signed-off-by: Michael Behr <mkbehr@google.com>

mkbehr requested review from lizan and zuercher as code owners July 23, 2018 23:07

lizan reviewed Jul 23, 2018

View reviewed changes

mkbehr added 2 commits July 24, 2018 13:26

Use RELEASE_ASSERT in tests that aren't ready to propagate timeout er…

d7cb98e

…rors. Signed-off-by: Michael Behr <mkbehr@google.com>

Merge remote-tracking branch 'upstream/master' into fake-upstream-tim…

b2012c7

…eout Signed-off-by: Michael Behr <mkbehr@google.com>

mkbehr added 2 commits July 25, 2018 12:48

Bugfixes for fake_upstream timeouts

5d26fca

RELEASE_ASSERT in AutonomousUpstream::createNetworkFilterChain; ABSL_MUST_USE_RESULT on FakeRawConnection::initialize Signed-off-by: Michael Behr <mkbehr@google.com>

Merge remote-tracking branch 'upstream/master' into fake-upstream-tim…

ce59cf0

…eout Signed-off-by: Michael Behr <mkbehr@google.com>

mkbehr added 3 commits July 26, 2018 14:57

Missing VERIFY_ASSERTION in FakeUpstream::waitForRawConnection

22fe837

Signed-off-by: Michael Behr <mkbehr@google.com>

Merge remote-tracking branch 'upstream/master' into fake-upstream-tim…

20521f4

…eout Signed-off-by: Michael Behr <mkbehr@google.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy into fak…

d1c829b

…e-upstream-timeout Signed-off-by: Michael Behr <mkbehr@google.com>

mattklein123 assigned zuercher Jul 28, 2018

zuercher reviewed Jul 30, 2018

View reviewed changes

mkbehr added 3 commits July 30, 2018 17:13

Merge branch 'master' of https://github.com/envoyproxy/envoy into fak…

a92ce84

…e-upstream-timeout Signed-off-by: Michael Behr <mkbehr@google.com>

Code review changes

f2a8bb4

- Static constant for default timeouts - do while false for VERIFY_ASSERTION - Reference for output arguments in waitForNewStream et al. Signed-off-by: Michael Behr <mkbehr@google.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy into fak…

45fe22f

…e-upstream-timeout Signed-off-by: Michael Behr <mkbehr@google.com>

zuercher approved these changes Jul 31, 2018

View reviewed changes

mattklein123 approved these changes Jul 31, 2018

View reviewed changes

mattklein123 merged commit 6a8b843 into envoyproxy:master Jul 31, 2018

Conversation

mkbehr commented Jul 23, 2018

Uh oh!

lizan Jul 23, 2018

Choose a reason for hiding this comment

Uh oh!

mkbehr Jul 24, 2018

Choose a reason for hiding this comment

Uh oh!

zuercher Jul 24, 2018

Choose a reason for hiding this comment

Uh oh!

mkbehr Jul 25, 2018

Choose a reason for hiding this comment

Uh oh!

mkbehr commented Jul 24, 2018

Uh oh!

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

Uh oh!

Uh oh!

mkbehr commented Jul 25, 2018

Uh oh!

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

Uh oh!

Uh oh!

mkbehr commented Jul 25, 2018

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

zuercher Jul 30, 2018

Choose a reason for hiding this comment

Uh oh!

mkbehr Jul 31, 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!

zuercher Jul 30, 2018

Choose a reason for hiding this comment

Uh oh!

zuercher Jul 30, 2018

Choose a reason for hiding this comment

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zuercher commented Jul 24, 2018 •

edited

Loading

zuercher commented Jul 25, 2018 •

edited

Loading

mkbehr Jul 31, 2018 •

edited

Loading