test: add optional timeouts to waiting for counters/gauges #12251

samflattery · 2020-07-23T15:12:05Z

Signed-off-by: Sam Flattery samflattery@google.com

Commit Message: Add timeouts to waiting for counters/gauges
Additional Description:

change waitForCounterEq/Ge to use the same functions as waitForGaugeEq/Ge
add a default timeout of 10s
raise an exception if they timeout

Risk Level: Low
Testing: passes xDS fuzzer tests which make heavy use of these
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Sam Flattery <samflattery@google.com>

samflattery · 2020-07-23T15:13:38Z

/cc @asraa @htuch

asraa

Thanks! I think moving to an AssertionResult will end up better than throwing an exception just for test clarity.

asraa · 2020-07-23T15:20:51Z

test/integration/server.h

  }

  void waitForCounterGe(const std::string& name, uint64_t value) override {
-    notifyingStatsAllocator().waitForCounterFromStringGe(name, value);
+    TestUtility::waitForCounterGe(statStore(), name, value, time_system_);


Somewhere here we should warn about the default timeout in the comments, because it's opaque that it might throw an exception.

I think it might make more sense to return an AssertionResult instead of throwing, but calls to these waitFor's will need to be wrapped with ASSERT_TRUE

That makes sense. Might be better to split this up into two functions, say waitForCounterGe and waitForCounterGeTimeout since there are dozens of calls to these across all the different tests. The fuzz test is probably the only one that would expect these functions to timeout, say if there is a bug in xDS and the update doesn't properly go through, since mostly they're used to wait for the server to catch up to updates in integration tests. What do you think?

I think that without ABSL_MUST_USE_RESULT you can ignore the AssertionResult.

Oh right sure, I thought you meant any calls to waitFor would need to be wrapped with ASSERT_TRUE, but I can just do it in the fuzzer. I'll make that change, thanks!

edit: I just moved the assert true to the server stub instead so that all calls will use it

test/test_common/utility.h

Signed-off-by: Sam Flattery <samflattery@google.com>

test/test_common/utility.cc

htuch · 2020-07-23T23:11:06Z

test/test_common/utility.h


  /**
   * Wait for a gauge to >= a given value.
   * @param store supplies the stats store.
   * @param name gauge name.
   * @param value target value.
   * @param time_system the time system to use for waiting.
+   * @param timeout the maximum time to wait before timing out.


Is there a way to prevent this from using real time? For tests I think we can only safely do this with simulated time. Tests already have a bounded lifetime, so we can rely on that for the real time counter wait timeout.

The main reason for doing this was that the real timeout is something like 5 minutes so debugging is kind of difficult when you have to wait so long to see the logs, etc. I can check to see if I can just condition on whether time_system_ is simulated time or not, but not sure how useful a timeout in simulated time would be. What do you think?

per offline discussion sam's changing the timeout to an optional, so users will fail on timeout only when a timeout param is passed

Signed-off-by: Sam Flattery <samflattery@google.com>

samflattery · 2020-07-28T11:34:13Z

@asraa I made two different implementations (see last two commits), one in which the user has to pass in the timeout every time, and one in which it is a member variable that can be set in the HttpIntegrationTest before the server is started. The second approach is a bit more complex though and involves passing the timeout through several functions before setting it so it's a bit more disruptive to the existing codebase. Happy to go with either, let me know what you think!

samflattery · 2020-07-28T12:50:51Z

Failing extension discovery test which Asra said was flaking

Signed-off-by: Sam Flattery <samflattery@google.com>

test/integration/server_stats.h

test/test_common/utility.h

Signed-off-by: Sam Flattery <samflattery@google.com>

asraa

Thanks!

samflattery · 2020-07-28T15:49:44Z

@asraa Still failing extension discovery after merging master, passes locally on --runs_per_test=100

asraa · 2020-07-28T15:57:17Z

#12326
I'll re-run

Signed-off-by: Sam Flattery <samflattery@google.com>

samflattery · 2020-07-29T10:33:37Z

Failing flaking extension discovery service test

asraa · 2020-07-29T18:50:18Z

Needs a merge :(

asraa

Thanks!

…y#12251) Commit Message: Add timeouts to waiting for counters/gauges in integration tests add a default timeout of 0s which will not enable the timeout for the existing tests raise an assertionfailure if they timeout Signed-off-by: Sam Flattery <samflattery@google.com>

…y#12251) Commit Message: Add timeouts to waiting for counters/gauges in integration tests add a default timeout of 0s which will not enable the timeout for the existing tests raise an assertionfailure if they timeout Signed-off-by: Sam Flattery <samflattery@google.com> Signed-off-by: chaoqinli <chaoqinli@google.com>

add timeouts to waiting for counters/gauges

71d789a

Signed-off-by: Sam Flattery <samflattery@google.com>

asraa suggested changes Jul 23, 2020

View reviewed changes

asraa self-assigned this Jul 23, 2020

Sam Flattery added 2 commits July 23, 2020 17:19

use AssertionResult instead of throwing exceptions

18dcf08

Signed-off-by: Sam Flattery <samflattery@google.com>

style fixes

adcff20

Signed-off-by: Sam Flattery <samflattery@google.com>

asraa reviewed Jul 23, 2020

View reviewed changes

test/test_common/utility.cc Outdated Show resolved Hide resolved

htuch reviewed Jul 23, 2020

View reviewed changes

remove log statements

6a7c3a7

Signed-off-by: Sam Flattery <samflattery@google.com>

asraa added the waiting:any label Jul 27, 2020

Sam Flattery added 2 commits July 28, 2020 11:50

impl with default parameters everywhere

b4a2ba4

Signed-off-by: Sam Flattery <samflattery@google.com>

impl with member variable

32b6fd2

Signed-off-by: Sam Flattery <samflattery@google.com>

repokitteh-read-only bot removed the waiting:any label Jul 28, 2020

Sam Flattery added 3 commits July 28, 2020 14:54

change back to passing defaults with 0 as no timeout

c702585

Signed-off-by: Sam Flattery <samflattery@google.com>

Merge branch 'master' into wait_timeout

6982019

add timeouts to xds_fuzz.{cc, h}

a2e2840

Signed-off-by: Sam Flattery <samflattery@google.com>

asraa reviewed Jul 28, 2020

View reviewed changes

test/integration/server_stats.h Show resolved Hide resolved

test/test_common/utility.h Outdated Show resolved Hide resolved

add documentation for params, add defaults to TestUtility

f1e9a0c

Signed-off-by: Sam Flattery <samflattery@google.com>

asraa previously approved these changes Jul 28, 2020

View reviewed changes

asraa changed the title ~~test: add timeouts to waiting for counters/gauges~~ test: add optional timeouts to waiting for counters/gauges Jul 28, 2020

Merge branch 'master' into wait_timeout

f7420a7

samflattery dismissed asraa’s stale review via f7420a7 July 29, 2020 08:53

Sam Flattery added 2 commits July 29, 2020 09:56

fix 100 line limit after resolving merge conflict on github

89a37c7

Signed-off-by: Sam Flattery <samflattery@google.com>

fix style

f25785d

Signed-off-by: Sam Flattery <samflattery@google.com>

asraa previously approved these changes Jul 29, 2020

View reviewed changes

Merge branch 'master' into wait_timeout

ef77159

samflattery dismissed asraa’s stale review via ef77159 July 30, 2020 08:41

asraa approved these changes Jul 30, 2020

View reviewed changes

asraa merged commit 702f1fb into envoyproxy:master Jul 30, 2020

samflattery deleted the wait_timeout branch July 31, 2020 09:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add optional timeouts to waiting for counters/gauges #12251

test: add optional timeouts to waiting for counters/gauges #12251

samflattery commented Jul 23, 2020

samflattery commented Jul 23, 2020

asraa left a comment

asraa Jul 23, 2020

samflattery Jul 23, 2020

asraa Jul 23, 2020

samflattery Jul 23, 2020

samflattery Jul 23, 2020

htuch Jul 23, 2020

samflattery Jul 24, 2020

asraa Jul 27, 2020

samflattery commented Jul 28, 2020 •

edited

Loading

samflattery commented Jul 28, 2020

asraa left a comment

samflattery commented Jul 28, 2020

asraa commented Jul 28, 2020

samflattery commented Jul 29, 2020

asraa commented Jul 29, 2020

asraa left a comment

test: add optional timeouts to waiting for counters/gauges #12251

test: add optional timeouts to waiting for counters/gauges #12251

Conversation

samflattery commented Jul 23, 2020

samflattery commented Jul 23, 2020

asraa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samflattery commented Jul 28, 2020 • edited Loading

samflattery commented Jul 28, 2020

asraa left a comment

Choose a reason for hiding this comment

samflattery commented Jul 28, 2020

asraa commented Jul 28, 2020

samflattery commented Jul 29, 2020

asraa commented Jul 29, 2020

asraa left a comment

Choose a reason for hiding this comment

samflattery commented Jul 28, 2020 •

edited

Loading