[base][test] Support no-args callbacks in TestFuture

A glaring omission of TestFuture was that you could not use it for
no-args callbacks, mainly due to the `using FirstType = ...` statement.

This forced people to use `TestFuture<bool>` to wait for things like
when a no-args observer method is invoked.

With this change `TestFuture<void>` can be used instead.

Bug: 1379290
Test: unit_tests --gtest_filter="*TestFuture*"
Change-Id: If742247df2b72d4e5d5577415ae443e7362db16d
Cq-Include-Trybots: luci.chrome.try:linux-chromeos-chrome
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3974451
Reviewed-by: Wez <wez@chromium.org>
Commit-Queue: Jeroen Dhollander <jeroendh@google.com>
Reviewed-by: Roland Bock <rbock@google.com>
Reviewed-by: Ben Franz <bfranz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1067457}

base/test/repeating_test_future.h

@@ -18,14 +18,14 @@
 
 namespace base::test {
 
-// Related class to |base::test::TestFuture|, which allows its callback and
+// Related class to `base::test::TestFuture`, which allows its callback and
 // AddValue() method to be called multiple times.
 //
 // Each call to Take() will return one element in FIFO fashion.
 // If no element is available, Take() will wait until an element becomes
 // available.
 //
-// Just like |base::test::TestFuture|, |base::test::RepeatingTestFuture| also
+// Just like `base::test::TestFuture`, `base::test::RepeatingTestFuture` also
 // supports callbacks which take multiple values. If this is the case Take()
 // will return a tuple containing all values passed to the callback.
 //
@@ -36,12 +36,12 @@ namespace base::test {
 //
 //     InvokeCallbackAsyncTwice(future.GetCallback());
 //
-//     ResultType first_result = future.Get();
-//     ResultType second_result = future.Get();
+//     ResultType first_result = future.Take();
+//     ResultType second_result = future.Take();
 //
 //     // When you come here, InvokeCallbackAsyncTwice has finished,
-//     // |first_result| contains the value passed to the first invocation
-//     // of the callback, and |second_result| has the result of the second
+//     // `first_result` contains the value passed to the first invocation
+//     // of the callback, and `second_result` has the result of the second
 //     // invocation.
 //   }
 //
@@ -78,7 +78,6 @@ template <typename... Types>
 class RepeatingTestFuture {
  public:
   using TupleType = std::tuple<std::decay_t<Types>...>;
-  using FirstType = typename std::tuple_element<0, TupleType>::type;
 
   RepeatingTestFuture() = default;
   RepeatingTestFuture(const RepeatingTestFuture&) = delete;
@@ -118,7 +117,7 @@ class RepeatingTestFuture {
   // and unblock any waiters.
   // This method is templated so you can specify how you need the arguments to
   // be passed - be it const, as reference, or anything you can think off.
-  // By default the callback accepts the arguments as |Types...|.
+  // By default the callback accepts the arguments as `Types...`.
   //
   // Example usage:
   //
@@ -164,7 +163,7 @@ class RepeatingTestFuture {
   //
   // Will DCHECK if a timeout happens.
   template <typename T = TupleType, internal::EnableIfSingleValue<T> = true>
-  FirstType Take() {
+  auto Take() {
     return std::get<0>(TakeTuple());
   }
 

base/test/repeating_test_future_unittest.cc

@@ -18,7 +18,7 @@ namespace {
 struct MoveOnlyValue {
  public:
   MoveOnlyValue() = default;
-  MoveOnlyValue(std::string data) : data(std::move(data)) {}
+  explicit MoveOnlyValue(std::string data) : data(std::move(data)) {}
   MoveOnlyValue(const MoveOnlyValue&) = delete;
   auto& operator=(const MoveOnlyValue&) = delete;
   MoveOnlyValue(MoveOnlyValue&&) = default;
@@ -79,7 +79,7 @@ TEST_F(RepeatingTestFutureTest,
   EXPECT_FALSE(future.IsEmpty());
 }
 
-TEST_F(RepeatingTestFutureTest, ShouldBeAbleToTakeElementsFiFo) {
+TEST_F(RepeatingTestFutureTest, ShouldTakeElementsFiFo) {
   RepeatingTestFuture<std::string> future;
 
   future.AddValue("first value");
@@ -122,7 +122,7 @@ TEST_F(RepeatingTestFutureTest,
 TEST_F(RepeatingTestFutureTest, WaitShouldReturnFalseIfTimeoutHappens) {
   test::ScopedRunLoopTimeout timeout(FROM_HERE, Milliseconds(1));
 
-  // |ScopedRunLoopTimeout| will automatically fail the test when a timeout
+  // `ScopedRunLoopTimeout` will automatically fail the test when a timeout
   // happens, so we use EXPECT_FATAL_FAILURE to handle this failure.
   // EXPECT_FATAL_FAILURE only works on static objects.
   static bool success;

base/test/test_future.h

@@ -49,7 +49,7 @@ namespace base::test {
 //
 //     const ResultType& actual_result = future.Get();
 //
-//     // When you come here, DoSomethingAsync has finished and |actual_result|
+//     // When you come here, DoSomethingAsync has finished and `actual_result`
 //     // contains the result passed to the callback.
 //   }
 //
@@ -69,6 +69,17 @@ namespace base::test {
 //     const std::string& second_argument = future.Get<1>();
 //   }
 //
+// Example if the callback has zero arguments:
+//
+//   TEST_F(MyTestFixture, MyTest) {
+//     TestFuture<void> signal;
+//
+//     object_under_test.DoSomethingAsync(signal.GetCallback());
+//
+//     EXPECT_TRUE(signal.Wait());
+//     // When you come here you know the async code is ready.
+//   }
+//
 // Or an example using TestFuture::Wait():
 //
 //   TEST_F(MyTestFixture, MyWaitTest) {
@@ -89,14 +100,16 @@ template <typename... Types>
 class TestFuture {
  public:
   using TupleType = std::tuple<std::decay_t<Types>...>;
-  using FirstType = typename std::tuple_element<0, TupleType>::type;
+
+  static_assert(std::tuple_size<TupleType>::value > 0,
+                "Don't use TestFuture<> but use TestFuture<void> instead");
 
   TestFuture() = default;
   TestFuture(const TestFuture&) = delete;
   TestFuture& operator=(const TestFuture&) = delete;
   ~TestFuture() = default;
 
-  // Wait for the value to arrive.
+  // Waits for the value to arrive.
   //
   // Returns true if the value arrived, or false if a timeout happens.
   //
@@ -133,7 +146,9 @@ class TestFuture {
   //   int first = future.Get<0>();
   //   std::string second = future.Get<1>();
   //
-  template <std::size_t I>
+  template <std::size_t I,
+            typename T = TupleType,
+            internal::EnableIfOneOrMoreValues<T> = true>
   const auto& Get() {
     return std::get<I>(GetTuple());
   }
@@ -156,7 +171,7 @@ class TestFuture {
   // Returns a callback that when invoked will store all the argument values,
   // and unblock any waiters.
   // Templated so you can specify how you need the arguments to be passed -
-  // const, reference, .... Defaults to simply |Types...|.
+  // const, reference, .... Defaults to simply `Types...`.
   //
   // Example usage:
   //
@@ -184,7 +199,7 @@ class TestFuture {
     return GetCallback<Types...>();
   }
 
-  // Set the value of the future.
+  // Sets the value of the future.
   // This will unblock any pending Wait() or Get() call.
   // This can only be called once.
   void SetValue(Types... values) {
@@ -193,7 +208,7 @@ class TestFuture {
     DCHECK(!values_.has_value())
         << "The value of a TestFuture can only be set once. If you need to "
            "handle an ordered stream of result values, use "
-           "|base::test::RepeatingTestFuture|.";
+           "`base::test::RepeatingTestFuture`.";
 
     values_ = std::make_tuple(std::forward<Types>(values)...);
     run_loop_.Quit();
@@ -203,35 +218,35 @@ class TestFuture {
   //  Accessor methods only available if the future holds a single value.
   //////////////////////////////////////////////////////////////////////////////
 
-  // Wait for the value to arrive, and returns its value.
+  // Waits for the value to arrive, and returns its value.
   //
   // Will DCHECK if a timeout happens.
   template <typename T = TupleType, internal::EnableIfSingleValue<T> = true>
-  [[nodiscard]] const FirstType& Get() {
+  [[nodiscard]] const auto& Get() {
     return std::get<0>(GetTuple());
   }
 
-  // Wait for the value to arrive, and move it out.
+  // Waits for the value to arrive, and move it out.
   //
   // Will DCHECK if a timeout happens.
   template <typename T = TupleType, internal::EnableIfSingleValue<T> = true>
-  [[nodiscard]] FirstType Take() {
+  [[nodiscard]] auto Take() {
     return std::get<0>(TakeTuple());
   }
 
   //////////////////////////////////////////////////////////////////////////////
   //  Accessor methods only available if the future holds multiple values.
   //////////////////////////////////////////////////////////////////////////////
 
-  // Wait for the values to arrive, and returns a tuple with the values.
+  // Waits for the values to arrive, and returns a tuple with the values.
   //
   // Will DCHECK if a timeout happens.
   template <typename T = TupleType, internal::EnableIfMultiValue<T> = true>
   [[nodiscard]] const TupleType& Get() {
     return GetTuple();
   }
 
-  // Wait for the values to arrive, and move a tuple with the values out.
+  // Waits for the values to arrive, and move a tuple with the values out.
   //
   // Will DCHECK if a timeout happens.
   template <typename T = TupleType, internal::EnableIfMultiValue<T> = true>
@@ -263,6 +278,39 @@ class TestFuture {
   base::WeakPtrFactory<TestFuture<Types...>> weak_ptr_factory_{this};
 };
 
+// Specialization so you can use `TestFuture` to wait for a no-args callback.
+//
+// This specialization offers a subset of the methods provided on the base
+// `TestFuture`, as there is no value to be returned.
+template <>
+class TestFuture<void> {
+ public:
+  // Waits until the callback or `SetValue()` is invoked.
+  //
+  // Fails your test if a timeout happens, but you can check the return value
+  // to improve the error reported:
+  //
+  //   ASSERT_TRUE(future.Wait()) << "Detailed error message";
+  [[nodiscard]] bool Wait() { return implementation_.Wait(); }
+
+  // Waits until the callback or `SetValue()` is invoked.
+  void Get() { std::ignore = implementation_.Get(); }
+
+  // Returns true if the callback or `SetValue()` was invoked.
+  bool IsReady() const { return implementation_.IsReady(); }
+
+  // Returns a callback that when invoked will unblock any waiters.
+  base::OnceCallback<void()> GetCallback() {
+    return base::BindOnce(implementation_.GetCallback(), true);
+  }
+
+  // Indicates this `TestFuture` is ready, and unblocks any waiters.
+  void SetValue() { implementation_.SetValue(true); }
+
+ private:
+  TestFuture<bool> implementation_;
+};
+
 }  // namespace base::test
 
 #endif  // BASE_TEST_TEST_FUTURE_H_

base/test/test_future_internal.h

@@ -5,24 +5,26 @@
 #ifndef BASE_TEST_TEST_FUTURE_INTERNAL_H_
 #define BASE_TEST_TEST_FUTURE_INTERNAL_H_
 
-namespace base {
-namespace test {
+#include <tuple>
+#include <type_traits>
 
-namespace internal {
+namespace base::test::internal {
+
+// Helper to only implement a method if the future holds one or more values
+template <typename Tuple>
+using EnableIfOneOrMoreValues =
+    std::enable_if_t<(std::tuple_size<Tuple>::value > 0), bool>;
 
 // Helper to only implement a method if the future holds a single value
 template <typename Tuple>
 using EnableIfSingleValue =
-    std::enable_if_t<(std::tuple_size<Tuple>::value <= 1), bool>;
+    std::enable_if_t<(std::tuple_size<Tuple>::value == 1), bool>;
 
 // Helper to only implement a method if the future holds multiple values
 template <typename Tuple>
 using EnableIfMultiValue =
     std::enable_if_t<(std::tuple_size<Tuple>::value > 1), bool>;
 
-}  // namespace internal
-
-}  // namespace test
-}  // namespace base
+}  // namespace base::test::internal
 
 #endif  // BASE_TEST_TEST_FUTURE_INTERNAL_H_

base/test/test_future_unittest.cc

@@ -26,8 +26,6 @@ struct MoveOnlyValue {
  public:
   MoveOnlyValue() = default;
   explicit MoveOnlyValue(int data) : data(data) {}
-  MoveOnlyValue(const MoveOnlyValue&) = delete;
-  auto& operator=(const MoveOnlyValue&) = delete;
   MoveOnlyValue(MoveOnlyValue&&) = default;
   MoveOnlyValue& operator=(MoveOnlyValue&&) = default;
   ~MoveOnlyValue() = default;
@@ -90,7 +88,7 @@ TEST_F(TestFutureTest, WaitShouldReturnTrueWhenValueArrives) {
 TEST_F(TestFutureTest, WaitShouldReturnFalseIfTimeoutHappens) {
   base::test::ScopedRunLoopTimeout timeout(FROM_HERE, base::Milliseconds(1));
 
-  // |ScopedRunLoopTimeout| will automatically fail the test when a timeout
+  // `ScopedRunLoopTimeout` will automatically fail the test when a timeout
   // happens, so we use EXPECT_FATAL_FAILURE to handle this failure.
   // EXPECT_FATAL_FAILURE only works on static objects.
   static bool success;
@@ -159,7 +157,7 @@ TEST_F(TestFutureTest, ShouldOnlyAllowSetValueToBeCalledOnce) {
                            "The value of a TestFuture can only be set once.");
 }
 
-TEST_F(TestFutureTest, ShouldUnblockWhenSetValueIsInvoked) {
+TEST_F(TestFutureTest, ShouldSignalWhenSetValueIsInvoked) {
   const int expected_value = 111;
   TestFuture<int> future;
 
@@ -315,4 +313,46 @@ TEST_F(TestFutureTest, ShouldSupportMultipleCvRefTypes) {
   EXPECT_EQ(expected_third_value, std::get<2>(take_result));
 }
 
+using TestFutureWithoutValuesTest = TestFutureTest;
+
+TEST_F(TestFutureWithoutValuesTest, IsReadyShouldBeTrueWhenSetValueIsInvoked) {
+  TestFuture<void> future;
+
+  EXPECT_FALSE(future.IsReady());
+
+  future.SetValue();
+
+  EXPECT_TRUE(future.IsReady());
+}
+
+TEST_F(TestFutureWithoutValuesTest, WaitShouldUnblockWhenSetValueIsInvoked) {
+  TestFuture<void> future;
+
+  RunLater([&future]() { future.SetValue(); });
+
+  ASSERT_FALSE(future.IsReady());
+  std::ignore = future.Wait();
+  EXPECT_TRUE(future.IsReady());
+}
+
+TEST_F(TestFutureWithoutValuesTest, WaitShouldUnblockWhenCallbackIsInvoked) {
+  TestFuture<void> future;
+
+  RunLater(future.GetCallback());
+
+  ASSERT_FALSE(future.IsReady());
+  std::ignore = future.Wait();
+  EXPECT_TRUE(future.IsReady());
+}
+
+TEST_F(TestFutureWithoutValuesTest, GetShouldUnblockWhenCallbackIsInvoked) {
+  TestFuture<void> future;
+
+  RunLater(future.GetCallback());
+
+  ASSERT_FALSE(future.IsReady());
+  future.Get();
+  EXPECT_TRUE(future.IsReady());
+}
+
 }  // namespace base::test