[FileCheck][unittest] Use parameterized unittests

[thopre]

Summary

Use parameterized unit tests to test FileCheck ExpressionValue API
rather than hand written code.

[grimar]

Honestly, my impression is that the code on the left reads much easier. And it is 100 lines shorter.
I am not familar with things like ::testing::TestWithParam though, may be there is a benefit of using it
that is not obvious to me atm.

I wonder what do other people think.

llvm/unittests/Support/FileCheckTest.cpp

296

You can use INT64_MAX instead.
(INT32_MAX/UINT32_MAX are used in many places in LLVM).

358

UINT32_MAX?

418

You can return here to avoid else:

  ASSERT_THAT_EXPECTED(SignedActualValue, Succeeded());
  EXPECT_EQ(*SignedExpectedValue, *SignedActualValue);
  return;
}

[jhenderson]

Thanks for looking at this. This hasn't quite collapsed the code in the way that I imagined it would, so I'm happy for this idea to be abandoned if you don't think it is worthwhile. Otherwise, I'll take a look next week.

[thopre]

In D80703#2062854, @jhenderson wrote:

Thanks for looking at this. This hasn't quite collapsed the code in the way that I imagined it would, so I'm happy for this idea to be abandoned if you don't think it is worthwhile. Otherwise, I'll take a look next week.

It could probably be quite shorter if there was a way to keep some parameter templated in the fixture (namely the distinction between signed and unsigned values). Unfortunately I had errors when I tried that.

llvm/unittests/Support/FileCheckTest.cpp

TEST_F(FileCheckTest, FormatKindEqualityOperators) {
   EXPECT_FALSE(UnsignedFormat != ExpressionFormat::Kind::Unsigned);
   EXPECT_FALSE(UnsignedFormat == ExpressionFormat::Kind::HexLower);
   EXPECT_TRUE(UnsignedFormat != ExpressionFormat::Kind::HexLower);
   ExpressionFormat NoFormat(ExpressionFormat::Kind::NoFormat);
   EXPECT_TRUE(NoFormat == ExpressionFormat::Kind::NoFormat);
   EXPECT_FALSE(NoFormat != ExpressionFormat::Kind::NoFormat);
 }
 
-template <class T1, class T2>
-static Expected<ExpressionValue> doValueOperation(binop_eval_t Operation,
-                                                  T1 LeftValue, T2 RightValue) {
-  ExpressionValue LeftOperand(LeftValue);
-  ExpressionValue RightOperand(RightValue);
-  return Operation(LeftOperand, RightOperand);
-}
+const int64_t MinInt64 = std::numeric_limits<int64_t>::min();
 
-template <class T>
-static void expectValueEqual(ExpressionValue ActualValue, T ExpectedValue) {
-  EXPECT_EQ(ExpectedValue < 0, ActualValue.isNegative());
-  if (ExpectedValue < 0) {
-    Expected<int64_t> SignedActualValue = ActualValue.getSignedValue();
-    ASSERT_THAT_EXPECTED(SignedActualValue, Succeeded());
-    EXPECT_EQ(*SignedActualValue, static_cast<int64_t>(ExpectedValue));
-  } else {
-    Expected<uint64_t> UnsignedActualValue = ActualValue.getUnsignedValue();
-    ASSERT_THAT_EXPECTED(UnsignedActualValue, Succeeded());
-    EXPECT_EQ(*UnsignedActualValue, static_cast<uint64_t>(ExpectedValue));
-  }
+template <typename T> struct ExpressionValueValidGetterFixture {
+  ExpressionValue InitialValue{0};
+  T ExpectedValue;
+
+  void expectValue(Expected<T> (ExpressionValue::*Getter)() const) {
+    Expected<T> RawValue = (this->InitialValue.*Getter)();
+    ASSERT_THAT_EXPECTED(RawValue, Succeeded());
+    EXPECT_EQ(*RawValue, ExpectedValue);
   }
+};
 
-template <class T1, class T2, class TR>
-static void expectOperationValueResult(binop_eval_t Operation, T1 LeftValue,
-                                       T2 RightValue, TR ResultValue) {
-  Expected<ExpressionValue> OperationResult =
-      doValueOperation(Operation, LeftValue, RightValue);
-  ASSERT_THAT_EXPECTED(OperationResult, Succeeded());
-  expectValueEqual(*OperationResult, ResultValue);
-}
+struct ExpressionValueValidGetUnsignedFixture
+    : ExpressionValueValidGetterFixture<uint64_t>,
+      public ::testing::TestWithParam<std::tuple<ExpressionValue, uint64_t>> {
+  void SetUp() { std::tie(InitialValue, ExpectedValue) = GetParam(); }
+};
 
-template <class T1, class T2>
-static void expectOperationValueResult(binop_eval_t Operation, T1 LeftValue,
-                                       T2 RightValue) {
-  expectError<OverflowError>(
-      "overflow error",
-      doValueOperation(Operation, LeftValue, RightValue).takeError());
+TEST_P(ExpressionValueValidGetUnsignedFixture, ValidGetUnsigned) {
+  expectValue(&ExpressionValue::getUnsignedValue);
 }
 
-const int64_t MinInt64 = std::numeric_limits<int64_t>::min();
-const int64_t MaxInt64 = std::numeric_limits<int64_t>::max();
-
-TEST_F(FileCheckTest, ExpressionValueGetUnsigned) {
+INSTANTIATE_TEST_CASE_P(ValidUnsignedGetters,
+                        ExpressionValueValidGetUnsignedFixture,
+                        ::testing::Values(
                             // Test positive value.
-  Expected<uint64_t> UnsignedValue = ExpressionValue(10).getUnsignedValue();
-  ASSERT_THAT_EXPECTED(UnsignedValue, Succeeded());
-  EXPECT_EQ(*UnsignedValue, 10U);
+                            std::make_tuple(10, 10),
 
                             // Test 0.
-  UnsignedValue = ExpressionValue(0).getUnsignedValue();
-  ASSERT_THAT_EXPECTED(UnsignedValue, Succeeded());
-  EXPECT_EQ(*UnsignedValue, 0U);
-
-  // Test max positive value.
-  UnsignedValue = ExpressionValue(MaxUint64).getUnsignedValue();
-  ASSERT_THAT_EXPECTED(UnsignedValue, Succeeded());
-  EXPECT_EQ(*UnsignedValue, MaxUint64);
+                            std::make_tuple(0, 0),
+
+                            // Test max uint64_t.
+                            std::make_tuple(MaxUint64, MaxUint64)), );
+
+template <typename T> struct ExpressionValueInvalidGetterFixture {
+  ExpressionValue InitialValue{0};
+  void expectGetterOverflow(Expected<T> (ExpressionValue::*Getter)() const) {
+    expectError<OverflowError>("overflow error",
+                               (this->InitialValue.*Getter)().takeError());
+  }
+};
+
+struct ExpressionValueInvalidGetUnsignedFixture
+    : ExpressionValueInvalidGetterFixture<uint64_t>,
+      public ::testing::TestWithParam<ExpressionValue> {
+  void SetUp() { InitialValue = GetParam(); }
+};
+
+TEST_P(ExpressionValueInvalidGetUnsignedFixture, InvalidGetUnsigned) {
+  expectGetterOverflow(&ExpressionValue::getUnsignedValue);
+}
 
+INSTANTIATE_TEST_CASE_P(InvalidUnsignedGetters,
+                        ExpressionValueInvalidGetUnsignedFixture,
+                        ::testing::Values(
                             // Test failure with negative value.
-  expectError<OverflowError>(
-      "overflow error", ExpressionValue(-1).getUnsignedValue().takeError());
+                            -1,
 
                             // Test failure with min negative value.
-  expectError<OverflowError>(
-      "overflow error",
-      ExpressionValue(MinInt64).getUnsignedValue().takeError());
+                            MinInt64), );
+
+const int64_t MaxInt64 = std::numeric_limits<int64_t>::max();
+
+struct ExpressionValueValidGetSignedFixture
+    : ExpressionValueValidGetterFixture<int64_t>,
+      public ::testing::TestWithParam<std::tuple<ExpressionValue, int64_t>> {
+  void SetUp() { std::tie(InitialValue, ExpectedValue) = GetParam(); }
+};
+
+TEST_P(ExpressionValueValidGetSignedFixture, ValidGetSigned) {
+  expectValue(&ExpressionValue::getSignedValue);
 }
 
-TEST_F(FileCheckTest, ExpressionValueGetSigned) {
+INSTANTIATE_TEST_CASE_P(ValidSignedGetters,
+                        ExpressionValueValidGetSignedFixture,
+                        ::testing::Values(
                             // Test positive value.
-  Expected<int64_t> SignedValue = ExpressionValue(10).getSignedValue();
-  ASSERT_THAT_EXPECTED(SignedValue, Succeeded());
-  EXPECT_EQ(*SignedValue, 10);
+                            std::make_tuple(10, 10),
 
                             // Test 0.
-  SignedValue = ExpressionValue(0).getSignedValue();
-  ASSERT_THAT_EXPECTED(SignedValue, Succeeded());
-  EXPECT_EQ(*SignedValue, 0);
+                            std::make_tuple(0, 0),
 
                             // Test max int64_t.
-  SignedValue = ExpressionValue(MaxInt64).getSignedValue();
-  ASSERT_THAT_EXPECTED(SignedValue, Succeeded());
-  EXPECT_EQ(*SignedValue, MaxInt64);
+                            std::make_tuple(MaxInt64, MaxInt64),
+
+                            // Test negative value.
+                            std::make_tuple(-1, -1), std::make_tuple(-10, -10),
+
+                            // Test min int64_t.
+                            std::make_tuple(MinInt64, MinInt64)), );
+
+struct ExpressionValueInvalidGetSignedFixture
+    : ExpressionValueInvalidGetterFixture<int64_t>,
+      public ::testing::TestWithParam<ExpressionValue> {
+  void SetUp() { InitialValue = GetParam(); }
+};
+
+TEST_P(ExpressionValueInvalidGetSignedFixture, InvalidGetSigned) {
+  expectGetterOverflow(&ExpressionValue::getSignedValue);
+}
 
+INSTANTIATE_TEST_CASE_P(InvalidSignedGetters,
+                        ExpressionValueInvalidGetSignedFixture,
+                        ::testing::Values(
                             // Test failure with too big positive value.
-  expectError<OverflowError>(
-      "overflow error", ExpressionValue(static_cast<uint64_t>(MaxInt64) + 1)
-                            .getSignedValue()
-                            .takeError());
+                            static_cast<uint64_t>(MaxInt64) + 1,
 
                             // Test failure with max uint64_t.
-  expectError<OverflowError>(
-      "overflow error",
-      ExpressionValue(MaxUint64).getSignedValue().takeError());
+                            MaxUint64), );
 
-  // Test negative value.
-  SignedValue = ExpressionValue(-10).getSignedValue();
-  ASSERT_THAT_EXPECTED(SignedValue, Succeeded());
-  EXPECT_EQ(*SignedValue, -10);
+struct ExpressionValueResultFixtureBase {
+  ExpressionValue ExpectedResult{0};
 
-  // Test min int64_t.
-  SignedValue = ExpressionValue(MinInt64).getSignedValue();
-  ASSERT_THAT_EXPECTED(SignedValue, Succeeded());
-  EXPECT_EQ(*SignedValue, MinInt64);
+  void expectResult(ExpressionValue ActualResult) {
+    EXPECT_EQ(ExpectedResult.isNegative(), ActualResult.isNegative());
+    if (ExpectedResult.isNegative()) {
+      Expected<int64_t> SignedExpectedValue = ExpectedResult.getSignedValue();
+      ASSERT_THAT_EXPECTED(SignedExpectedValue, Succeeded());
+      Expected<int64_t> SignedActualValue = ActualResult.getSignedValue();
+      ASSERT_THAT_EXPECTED(SignedActualValue, Succeeded());
+      EXPECT_EQ(*SignedExpectedValue, *SignedActualValue);
+    } else {
+      Expected<uint64_t> UnsignedExpectedValue =
+          ExpectedResult.getUnsignedValue();
+      ASSERT_THAT_EXPECTED(UnsignedExpectedValue, Succeeded());
+      Expected<uint64_t> UnsignedActualValue = ActualResult.getUnsignedValue();
+      ASSERT_THAT_EXPECTED(UnsignedActualValue, Succeeded());
+      EXPECT_EQ(*UnsignedExpectedValue, *UnsignedActualValue);
+    }
+  }
+};
+
+struct ExpressionValueAbsoluteFixture
+    : ExpressionValueResultFixtureBase,
+      public ::testing::TestWithParam<
+          std::tuple<ExpressionValue, ExpressionValue>> {
+  void SetUp() { std::tie(InitialValue, ExpectedResult) = GetParam(); }
+
+  ExpressionValue InitialValue{0};
+};
+
+TEST_P(ExpressionValueAbsoluteFixture, Absolute) {
+  expectResult(InitialValue.getAbsolute());
 }
 
-TEST_F(FileCheckTest, ExpressionValueAbsolute) {
+INSTANTIATE_TEST_CASE_P(
+    Absolutes, ExpressionValueAbsoluteFixture,
+    ::testing::Values(
         // Test positive value.
-  expectValueEqual(ExpressionValue(10).getAbsolute(), 10);
+        std::make_tuple(10, 10),
 
         // Test 0.
-  expectValueEqual(ExpressionValue(0).getAbsolute(), 0);
+        std::make_tuple(0, 0),
 
         // Test max uint64_t.
-  expectValueEqual(ExpressionValue(MaxUint64).getAbsolute(), MaxUint64);
+        std::make_tuple(MaxUint64, MaxUint64),
 
         // Test negative value.
-  expectValueEqual(ExpressionValue(-10).getAbsolute(), 10);
+        std::make_tuple(-10, 10),
 
         // Test absence of overflow on min int64_t.
-  expectValueEqual(ExpressionValue(MinInt64).getAbsolute(),
-                   static_cast<uint64_t>(-(MinInt64 + 10)) + 10);
+        std::make_tuple(MinInt64,
+                        static_cast<uint64_t>(-(MinInt64 + 10)) + 10)), );
+
+struct ExpressionValueOperationFixtureBase {
+  ExpressionValue LeftOperand{0};
+  ExpressionValue RightOperand{0};
+
+  Expected<ExpressionValue> doOperation(binop_eval_t Operation) {
+    return Operation(LeftOperand, RightOperand);
+  }
+
+  void expectOperationOverflow(binop_eval_t Operation) {
+    expectError<OverflowError>("overflow error",
+                               doOperation(Operation).takeError());
+  }
+};
+
+struct ExpressionValueValidOperationFixture
+    : ExpressionValueOperationFixtureBase,
+      ExpressionValueResultFixtureBase,
+      public ::testing::TestWithParam<
+          std::tuple<ExpressionValue, ExpressionValue, ExpressionValue>> {
+  void SetUp() {
+    std::tie(LeftOperand, RightOperand, ExpectedResult) = GetParam();
+  }
+
+  void expectOperationResult(binop_eval_t Operation) {
+    Expected<ExpressionValue> OperationResult = doOperation(Operation);
+    ASSERT_THAT_EXPECTED(OperationResult, Succeeded());
+    expectResult(*OperationResult);
+  }
+};
+
+struct ExpressionValueValidAdditionFixture
+    : ExpressionValueValidOperationFixture {};
+
+TEST_P(ExpressionValueValidAdditionFixture, ValidAddition) {
+  expectOperationResult(operator+);
 }
 
-TEST_F(FileCheckTest, ExpressionValueAddition) {
+INSTANTIATE_TEST_CASE_P(ValidAdditions, ExpressionValueValidAdditionFixture,
+                        ::testing::Values(
                             // Test both negative values.
-  expectOperationValueResult(operator+, -10, -10, -20);
+                            std::make_tuple(-10, -10, -20),
 
-  // Test both negative values with underflow.
-  expectOperationValueResult(operator+, MinInt64, -1);
-  expectOperationValueResult(operator+, MinInt64, MinInt64);
+                            // Test negative and positive values.
+                            std::make_tuple(-10, 10, 0),
+                            std::make_tuple(-10, 11, 1),
+                            std::make_tuple(-11, 10, -1),
 
-  // Test negative and positive value.
-  expectOperationValueResult(operator+, -10, 10, 0);
-  expectOperationValueResult(operator+, -10, 11, 1);
-  expectOperationValueResult(operator+, -11, 10, -1);
-
-  // Test positive and negative value.
-  expectOperationValueResult(operator+, 10, -10, 0);
-  expectOperationValueResult(operator+, 10, -11, -1);
-  expectOperationValueResult(operator+, 11, -10, 1);
+                            // Test positive and negative values.
+                            std::make_tuple(10, -10, 0),
+                            std::make_tuple(10, -11, -1),
+                            std::make_tuple(11, -10, 1),
 
                             // Test both positive values.
-  expectOperationValueResult(operator+, 10, 10, 20);
+                            std::make_tuple(10, 10, 20)), );
 
-  // Test both positive values with overflow.
-  expectOperationValueResult(operator+, MaxUint64, 1);
-  expectOperationValueResult(operator+, MaxUint64, MaxUint64);
+struct ExpressionValueInvalidOperationFixture
+    : ExpressionValueOperationFixtureBase,
+      public ::testing::TestWithParam<
+          std::tuple<ExpressionValue, ExpressionValue>> {
+  void SetUp() { std::tie(LeftOperand, RightOperand) = GetParam(); }
+};
+
+struct ExpressionValueInvalidAdditionFixture
+    : ExpressionValueInvalidOperationFixture {};
+
+TEST_P(ExpressionValueInvalidAdditionFixture, InvalidAddition) {
+  expectOperationOverflow(operator+);
 }
 
-TEST_F(FileCheckTest, ExpressionValueSubtraction) {
-  // Test negative value and value bigger than int64_t max.
-  expectOperationValueResult(operator-, -10, MaxUint64);
+INSTANTIATE_TEST_CASE_P(InvalidAdditions, ExpressionValueInvalidAdditionFixture,
+                        ::testing::Values(
+                            // Test both negative values with underflow.
+                            std::make_tuple(MinInt64, -1),
+                            std::make_tuple(MinInt64, MinInt64),
+
+                            // Test both positive values with overflow.
+                            std::make_tuple(MaxUint64, 1),
+                            std::make_tuple(MaxUint64, MaxUint64)), );
 
-  // Test negative and positive value with underflow.
-  expectOperationValueResult(operator-, MinInt64, 1);
+struct ExpressionValueValidSubtractionFixture
+    : ExpressionValueValidOperationFixture {};
 
-  // Test negative and positive value.
-  expectOperationValueResult(operator-, -10, 10, -20);
+TEST_P(ExpressionValueValidSubtractionFixture, ValidSubtraction) {
+  expectOperationResult(operator-);
+}
+
+INSTANTIATE_TEST_CASE_P(
+    ValidSubtractions, ExpressionValueValidSubtractionFixture,
+    ::testing::Values(
+        // Test negative and positive values.
+        std::make_tuple(-10, 10, -20),
 
         // Test both negative values.
-  expectOperationValueResult(operator-, -10, -10, 0);
-  expectOperationValueResult(operator-, -11, -10, -1);
-  expectOperationValueResult(operator-, -10, -11, 1);
+        std::make_tuple(-10, -10, 0), std::make_tuple(-11, -10, -1),
+        std::make_tuple(-10, -11, 1),
 
         // Test positive and negative values.
-  expectOperationValueResult(operator-, 10, -10, 20);
+        std::make_tuple(10, -10, 20),
 
         // Test both positive values with result positive.
-  expectOperationValueResult(operator-, 10, 5, 5);
-
-  // Test both positive values with underflow.
-  expectOperationValueResult(operator-, 0, MaxUint64);
-  expectOperationValueResult(operator-, 0,
-                             static_cast<uint64_t>(-(MinInt64 + 10)) + 11);
+        std::make_tuple(10, 5, 5),
 
         // Test both positive values with result < -(max int64_t)
-  expectOperationValueResult(operator-, 10,
-                             static_cast<uint64_t>(MaxInt64) + 11,
-                             -MaxInt64 - 1);
+        std::make_tuple(10, static_cast<uint64_t>(MaxInt64) + 11,
+                        -MaxInt64 - 1),
 
         // Test both positive values with 0 > result > -(max int64_t)
-  expectOperationValueResult(operator-, 10, 11, -1);
+        std::make_tuple(10, 11, -1)), );
+
+struct ExpressionValueInvalidSubtractionFixture
+    : ExpressionValueInvalidOperationFixture {};
+
+TEST_P(ExpressionValueInvalidSubtractionFixture, InvalidSubtraction) {
+  expectOperationOverflow(operator-);
 }
 
+INSTANTIATE_TEST_CASE_P(
+    InvalidSubtractions, ExpressionValueInvalidSubtractionFixture,
+    ::testing::Values(
+        // Test negative value and value bigger than int64_t max.
+        std::make_tuple(-10, MaxUint64),
+
+        // Test negative and positive values with underflow.
+        std::make_tuple(MinInt64, 1),
+
+        // Test both positive values with underflow.
+        std::make_tuple(0, MaxUint64),
+        std::make_tuple(0, static_cast<uint64_t>(-(MinInt64 + 10)) + 11)), );
+
 TEST_F(FileCheckTest, Literal) {
   SourceMgr SM;
 
   // Eval returns the literal's value.
   ExpressionLiteral Ten(bufferize(SM, "10"), 10u);
   Expected<ExpressionValue> Value = Ten.eval();
   ASSERT_THAT_EXPECTED(Value, Succeeded());
   EXPECT_EQ(10, cantFail(Value->getSignedValue()));