Rework how tests deal with future timeout #496

var-const · 2021-06-26T01:52:05Z

Also fix a potential bug in DescribeFailedFuture (error_message() can return null).

github-actions · 2021-06-26T01:53:00Z

❌ Integration test FAILED

Requested by @var-const on commit e5b44cf
Last updated: Fri Jul 23 12:56 PDT 2021
View integration test log & download artifacts

Failures	Configs
missing_log	[TEST] [ERROR] [iOS] [macos] [simulator_target]
firestore	[TEST] [FAILURE] [Android] [macos] [emulator_target] (1 failed tests) TestDocumentSnapshotEventsForChange

dconeybe

Overall, this looks fantastic! It's great to see the timeout logic de-duplicated in several places. Thanks for doing this.

firestore/integration_test_internal/src/firestore_integration_test.h

dconeybe · 2021-06-29T15:19:00Z

firestore/integration_test_internal/src/firestore_integration_test.h

+// occurs. Returns `true` on success, `false` on timeout or if exit signal was
+// received.
+template <typename PredT>
+bool WaitUntil(const PredT& pred, int timeout_ms = kTimeOutMillis);


Consider returning an enum from WaitUntil() and WaitUntilFutureCompletes() instead of conflating timeout and exit signal with a false return value. This will improve readability at the call sites since it is not immediately obvious without reading the function's documentation or implementation what the return value means and will avoid misleading "timeout" error messages when the exit signal occurs.

I thought about this but was leaning towards not doing it. It seems to complicate the common case for the sake of a very rare occurrence. I'm not sure this even works correctly -- e.g. on Linux the integration tests seem to completely ignore Ctrl-C.

I'm not sure this even works correctly -- e.g. on Linux the integration tests seem to completely ignore Ctrl-C.

Please disregard -- this is actually working correctly. I'm still hesitant to complicate the common case, though.

Hmm, it seems like it actually works in some tests and doesn't work in others.

My concern still stands, that returning a bool detracts from readability at the call sites. And confounding exit signal with timeout could lead to misleading test failure messages. When tests timeout they are already hard enough to debug and unclear error messages can further make that harder. This doesn't block my approval of this PR though.

It seems to me that the root cause here is that these functions just don't have a great way of reporting this event, which really seems more akin to an exception. There also isn't much tests can do about it, so I'm not sure they need the ability to check for this condition. I think that this type of exit usually happens due to user input, so it shouldn't be confusing to the user, at least in the general case.

Maybe we should throw an exception now that exceptions are enabled in tests, especially once #561 is merged. In any case, that's out of scope for this PR so I'm fine with this code as-is.

dconeybe · 2021-06-29T15:20:31Z

firestore/integration_test_internal/src/firestore_integration_test.h

@@ -280,37 +285,27 @@ class FirestoreIntegrationTest : public testing::Test {

  // TODO(zxu): add a helper function to block on signal.

-  // A helper function to block until the future completes.
+  // Blocks until the future completes and returns the future result.


ultranit: Consider adding a comma after "completes".

Hmm, my understanding is that this is a compound predicate (Blocks... and returns) and thus doesn't require a comma. I can't dig up a very authoritative reference quickly but the few links that come up in a search seem to agree -- see e.g. this.

You may be technically correct. For me, every time I read this sentence I read "Blocks until the future completes and returns" which implies that the future "completes and returns" but "returns" actually gets associated with the text that follows. Totally up to you though :)

Ah, I see what you mean, and I fully agree. How about Blocks until the future is completed and returns...? I think it could still be read the wrong way but it seems like an improvement. What do you think?

Or, you could add a comma after "completes" ;) I think without the comma any wording has some ambiguity.

firestore/integration_test_internal/src/firestore_integration_test.h

dconeybe · 2021-07-23T14:55:30Z

firestore/integration_test_internal/src/util/future_test_util.h

-// This function is found dynamically by googletest to print a Future object
-// in a test failure message.
-void PrintTo(const Future<void>& future, std::ostream* os);
+// This function is via ADL by Googletest to print a Future object in a test


I think there is a missing verb in this sentence: "This function is via ADL...". Should it be "is found"?

dconeybe

Approved, with one very minor comment about a comment wording.

jonsimantov · 2021-12-22T22:37:42Z

@dconeybe Is this PR still relevant? Should it be merged or closed?

🍞 Dismissed stale approval on external PR.

dconeybe · 2022-01-04T17:16:00Z

Yes, it is still relevant. I've merged in HEAD and assigned to myself.

Initial commit

66f36f0

google-cla bot added the cla: yes label Jun 26, 2021

var-const added the tests-requested: quick Trigger a quick set of integration tests. label Jun 26, 2021

github-actions bot added tests: in-progress This PR's integration tests are in progress. and removed tests-requested: quick Trigger a quick set of integration tests. labels Jun 26, 2021

style

560d56d

github-actions bot added the tests: failed This PR's integration tests failed. label Jun 26, 2021

firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jun 26, 2021

var-const added 2 commits June 28, 2021 20:12

Fixes

7998543

style

c86d850

var-const added tests-requested: quick Trigger a quick set of integration tests. and removed tests: failed This PR's integration tests failed. labels Jun 29, 2021

github-actions bot added tests: in-progress This PR's integration tests are in progress. and removed tests-requested: quick Trigger a quick set of integration tests. labels Jun 29, 2021

var-const added 2 commits June 28, 2021 20:39

Simplify

3629636

Fix comment

4644cb6

var-const requested a review from dconeybe June 29, 2021 00:44

var-const assigned dconeybe Jun 29, 2021

var-const added the api: firestore label Jun 29, 2021

github-actions bot added the tests: failed This PR's integration tests failed. label Jun 29, 2021

var-const added tests-requested: full Trigger a FULL set of integration tests (uses expanded test matrix). and removed tests: failed This PR's integration tests failed. tests: in-progress This PR's integration tests are in progress. labels Jun 29, 2021

github-actions bot added tests: in-progress This PR's integration tests are in progress. tests: failed This PR's integration tests failed. and removed tests-requested: full Trigger a FULL set of integration tests (uses expanded test matrix). labels Jun 29, 2021

firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jun 29, 2021

dconeybe requested changes Jun 29, 2021

View reviewed changes

dconeybe removed their assignment Jun 29, 2021

var-const assigned dconeybe and unassigned var-const Jul 23, 2021

github-actions bot added the tests: failed This PR's integration tests failed. label Jul 23, 2021

firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jul 23, 2021

var-const added tests-requested: quick Trigger a quick set of integration tests. and removed tests: failed This PR's integration tests failed. labels Jul 23, 2021

github-actions bot added tests: in-progress This PR's integration tests are in progress. tests: failed This PR's integration tests failed. and removed tests-requested: quick Trigger a quick set of integration tests. labels Jul 23, 2021

firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jul 23, 2021

dconeybe reviewed Jul 23, 2021

View reviewed changes

dconeybe previously approved these changes Jul 23, 2021

View reviewed changes

dconeybe assigned var-const and unassigned dconeybe Jul 23, 2021

var-const added 2 commits July 23, 2021 14:26

Review

4684e79

Merge branch 'main' into varconst/await

80f6e63

var-const added tests-requested: quick Trigger a quick set of integration tests. and removed tests: failed This PR's integration tests failed. labels Jul 23, 2021

github-actions bot added tests: in-progress This PR's integration tests are in progress. tests: failed This PR's integration tests failed. and removed tests-requested: quick Trigger a quick set of integration tests. labels Jul 23, 2021

firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jul 23, 2021

Merge main branch at 0379ec4

dc10d7e

dconeybe assigned dconeybe and unassigned var-const Jan 4, 2022

auto-merge was automatically disabled May 5, 2022 03:14
Merge could not be authorized

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework how tests deal with future timeout #496

Rework how tests deal with future timeout #496

var-const commented Jun 26, 2021 •

edited

Loading

github-actions bot commented Jun 26, 2021 •

edited

Loading

dconeybe left a comment

dconeybe Jun 29, 2021

var-const Jun 30, 2021

var-const Jun 30, 2021

var-const Jul 1, 2021

dconeybe Jul 8, 2021

var-const Jul 13, 2021

dconeybe Jul 23, 2021

dconeybe Jun 29, 2021

var-const Jun 30, 2021

dconeybe Jul 7, 2021

var-const Jul 13, 2021

dconeybe Jul 13, 2021

dconeybe Jul 23, 2021

dconeybe left a comment

jonsimantov commented Dec 22, 2021

dconeybe commented Jan 4, 2022

Rework how tests deal with future timeout #496

Are you sure you want to change the base?

Rework how tests deal with future timeout #496

Conversation

var-const commented Jun 26, 2021 • edited Loading

github-actions bot commented Jun 26, 2021 • edited Loading

❌ Integration test FAILED

dconeybe 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

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

dconeybe left a comment

Choose a reason for hiding this comment

jonsimantov commented Dec 22, 2021

dconeybe commented Jan 4, 2022

var-const commented Jun 26, 2021 •

edited

Loading

github-actions bot commented Jun 26, 2021 •

edited

Loading