[libc] [UnitTest] Add Matchers

[abrachet]

Summary

This patch adds gtest-like matchers and EXPECT|ASSERT_THAT macros. It also adds matchers Succeeds and Fails and has examples using these in test/src/signal/sigaddset_test.cpp.

[gchatelet]

libc/utils/testutils/StreamWrapper.h

16

It would be nice to explain the purpose of this class.

[abrachet]

• Added comment explaining purpose of StreamWrapper
• Removed pointless default initialization of StreamWrapper::OS

[gchatelet]

libc/utils/testutils/StreamWrapper.h

16

Thx. There's a typo on indirection.

[abrachet]

Fix typo

[sivachandra]

libc/utils/UnitTest/Matchers.h

1 ↗

(On Diff #248115)

I didn't go look what gtest's suggested organization is, but instead of putting all matchers in Matchers.h, does it make sense to put the ErrnoSetterMacther class in its own header file?

16 ↗

(On Diff #248115)

Why not enclose in __llvm_libc::testing? To avoid verbosity?

48 ↗

(On Diff #248115)

These names are generic enough that some kind of specificity would be nice? I am not able to think of an alternate. May be ErrnoSetterSucceeds and ErrnoSetterFails?

libc/utils/testutils/StreamWrapper.h

2

I don't necessarily like this, but can't think of an alternate. So, LGTM.

[abrachet]

• Rename Matchers.h to ErrnoSetterMatcher.h
• Moved Fails and Succeeds behind namespace ErrnoSetterMatcher so they can be easily shorted with using
• Removed some newlines between EXPECT_THAT's in sigaddset_test.cpp
• Put ErrnoSetterMatcher in namespace __llvm_libc::testing

[abrachet]

libc/utils/UnitTest/ErrnoSetterMatcher.h

54

These names are generic enough that some kind of specificity would be nice? I am not able to think of an alternate. May be ErrnoSetterSucceeds and ErrnoSetterFails?

What about this set up? I think it is nice to be able to use a using and refer to these as just Succeeds and Fails for brevity. No strong preference happy to call these ErrnoSetterSucceeds/Fails

libc/utils/UnitTest/Matchers.h

16 ↗

(On Diff #248115)

Good call.

libc/utils/testutils/StreamWrapper.h

2

Agreed, it's not ideal, but I think it is required.

[sivachandra]

Couple of really nit-picky comments but overall LGTM.

libc/utils/UnitTest/ErrnoSetterMatcher.h

14

Normally, we should not use parts of the libc to build its testing infrastructure. This change is not doing anything of that sort, but having this include here gives an impression that we are using the libc to build the test infrastructure. So, may be add a comment of some kind to make it absolutely clear?

54

LGTM

57

Sorry I missed pointing out earlier: why not make the order of args to Fail and Succeeds the same? Even to the constructor of ErrnoSetterMatcher above?

[abrachet]

libc/utils/UnitTest/ErrnoSetterMatcher.h

14

Yes I was definitely weary about this. I had originally planned to put this file in test/ not utils/UnitTest but couldn't find a good place. Maybe making a common directory in test? So in libc/test/common/ErrnoSetterMatcher.h? Does that sound better?

57

I agree it is confusing.

The idea here was that for a failure errno is always different, and it would be awkward to do Fails(/*ExpectedReturn*/-1, EINVAL) rather than Fails(EINVAL). Likewise for Success(/*ExpectedErrno*/0, /*ExpectedReturn*/0) is awkward because errno should always be 0. Now that I think about it, maybe Success shouldn't take an errno parameter at all I can't imagine it ever being useful.

My plan is for these to be able to take matchers of their own in a future patch. I could in this patch have these take matchers and create an ErrnoIs/ReturnIs if you think the ordering of arguments is confusing enough to warrant this.

[sivachandra]

libc/utils/UnitTest/ErrnoSetterMatcher.h

14

No, leave it as is but add a comment. We want test/... to reflect the directory tree of the rest of the repo. If we really want a separate place, then may be utils/testmatchers. However, it feels like we should wait for more examples before we move things around.

57

SGTM. Worst case, we can easily fix this if it gets irritating in future.

[abrachet]

Added a comment explaining the inclusion of internal llvm-libc header in ErrnoSetterMatcher.h

[sivachandra]

libc/utils/UnitTest/ErrnoSetterMatcher.h

14

I am beginning to feel that your suggestion is more appropriate. For, the current arrangement breaks a layering rule: a part of utils depends on src. We do not want utils to depend on src.

To keep the directory structure of test match that of the the libc tree, we can take one of these two options:

1. Move testutils up out of utils in to a top level directory of its own.
2. Move just this file into test and not nest it under test/common.

I do not have an strong leanings, but it seems like #2 can be less confusing. WDYT?

[abrachet]

libc/utils/UnitTest/ErrnoSetterMatcher.h

14

No strong opinion either. I agree #2 is less confusing and slightly simpler I think.

libc/test/src/signal/sigaddset_test.cpp

 //===----------------------- Unittests for sigaddset ----------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "include/errno.h"
 #include "include/signal.h"
 #include "src/errno/llvmlibc_errno.h"
 #include "src/signal/sigaddset.h"
 
+#include "utils/UnitTest/ErrnoSetterMatcher.h"
 #include "utils/UnitTest/Test.h"
 
 // This tests invalid inputs and ensures errno is properly set.
 TEST(SignalTest, SigaddsetInvalid) {
-  llvmlibc_errno = 0;
-  EXPECT_EQ(__llvm_libc::sigaddset(nullptr, SIGSEGV), -1);
-  EXPECT_EQ(llvmlibc_errno, EINVAL);
+  using __llvm_libc::testing::ErrnoSetterMatcher::Fails;
+  using __llvm_libc::testing::ErrnoSetterMatcher::Succeeds;
+  EXPECT_THAT(__llvm_libc::sigaddset(nullptr, SIGSEGV), Fails(EINVAL));
 
   sigset_t sigset;
-  llvmlibc_errno = 0;
-  EXPECT_EQ(__llvm_libc::sigaddset(&sigset, -1), -1);
-  EXPECT_EQ(llvmlibc_errno, EINVAL);
+  EXPECT_THAT(__llvm_libc::sigaddset(&sigset, -1), Fails(EINVAL));
 
   // This doesn't use NSIG because __llvm_libc::sigaddset error checking is
   // against sizeof(sigset_t) not NSIG.
   constexpr int bitsInSigsetT = 8 * sizeof(sigset_t);
 
-  llvmlibc_errno = 0;
-  EXPECT_EQ(__llvm_libc::sigaddset(&sigset, bitsInSigsetT + 1), -1);
-  EXPECT_EQ(llvmlibc_errno, EINVAL);
-
-  llvmlibc_errno = 0;
-  EXPECT_EQ(__llvm_libc::sigaddset(&sigset, 0), -1);
-  EXPECT_EQ(llvmlibc_errno, EINVAL);
-
-  llvmlibc_errno = 0;
-  EXPECT_EQ(__llvm_libc::sigaddset(&sigset, bitsInSigsetT), 0);
-  EXPECT_EQ(llvmlibc_errno, 0);
+  EXPECT_THAT(__llvm_libc::sigaddset(&sigset, bitsInSigsetT + 1),
+              Fails(EINVAL));
+  EXPECT_THAT(__llvm_libc::sigaddset(&sigset, 0), Fails(EINVAL));
+  EXPECT_THAT(__llvm_libc::sigaddset(&sigset, bitsInSigsetT), Succeeds());
 }

libc/utils/UnitTest/CMakeLists.txt

 add_llvm_library(
   LibcUnitTest
   Test.cpp
   Test.h
+  ErrnoSetterMatcher.h
   LINK_COMPONENTS Support
 )
 target_include_directories(LibcUnitTest PUBLIC ${LIBC_SOURCE_DIR})
 add_dependencies(LibcUnitTest standalone_cpp)
 target_link_libraries(LibcUnitTest PUBLIC libc_test_utils)

libc/utils/UnitTest/ErrnoSetterMatcher.h

+//===--------------------- ErrnoSetterMatcher.h -----------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_UTILS_UNITTEST_ERRNOSETTERMATCHER_H
+#define LLVM_LIBC_UTILS_UNITTEST_ERRNOSETTERMATCHER_H
+
+#include "Test.h"
+
+// Using LLVM libc headers in UnitTest is not ideal however we also want the
+// test/ directory to have the same layout as libc/ so there is no clean place
+// to put this file except for in utils/UnitTest/.
+#include "src/errno/llvmlibc_errno.h"
+
+namespace __llvm_libc {
+namespace testing {
+
+namespace internal {
+
+extern "C" const char *strerror(int);
+
+template <typename T> class ErrnoSetterMatcher : public Matcher<T> {
+  T ExpectedReturn;
+  T ActualReturn;
+  int ExpectedErrno;
+  int ActualErrno;
+
+public:
+  ErrnoSetterMatcher(T ExpectedReturn, int ExpectedErrno)
+      : ExpectedReturn(ExpectedReturn), ExpectedErrno(ExpectedErrno) {}
+
+  void explainError(testutils::StreamWrapper &OS) override {
+    if (ActualReturn != ExpectedReturn)
+      OS << "Expected return to be " << ExpectedReturn << " but got "
+         << ActualReturn << ".\nExpecte errno to be " << strerror(ExpectedErrno)
+         << " but got " << strerror(ActualErrno) << ".\n";
+    else
+      OS << "Correct value " << ExpectedReturn
+         << " was returned\nBut errno was unexpectely set to "
+         << strerror(ActualErrno) << ".\n";
+  }
+
+  bool match(T Got) {
+    ActualReturn = Got;
+    ActualErrno = llvmlibc_errno;
+    llvmlibc_errno = 0;
+    return Got == ExpectedReturn && ActualErrno == ExpectedErrno;
+  }
+};
+
+} // namespace internal
+
+namespace ErrnoSetterMatcher {
+
+template <typename RetT = int>
+static internal::ErrnoSetterMatcher<RetT> Succeeds(RetT ExpectedReturn = 0,
+                                                   int ExpectedErrno = 0) {
+  return {ExpectedReturn, ExpectedErrno};
+}
+
+template <typename RetT = int>
+static internal::ErrnoSetterMatcher<RetT> Fails(int ExpectedErrno,
+                                                RetT ExpectedReturn = -1) {
+  return {ExpectedReturn, ExpectedErrno};
+}
+
+} // namespace ErrnoSetterMatcher
+
+} // namespace testing
+} // namespace __llvm_libc
+
+#endif // LLVM_LIBC_UTILS_UNITTEST_ERRNOSETTERMATCHER_H

libc/utils/UnitTest/Test.h

 //===------------------ Base class for libc unittests -----------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_LIBC_UTILS_UNITTEST_H
 #define LLVM_LIBC_UTILS_UNITTEST_H
 
 // This file can only include headers from utils/CPP/ or utils/testutils. No
 // other headers should be included.
 
 #include "utils/CPP/TypeTraits.h"
 #include "utils/testutils/ExecuteFunction.h"
+#include "utils/testutils/StreamWrapper.h"
 
 namespace __llvm_libc {
 namespace testing {
 
 class RunContext;
 
 // Only the following conditions are supported. Notice that we do not have
 // a TRUE or FALSE condition. That is because, C library funtions do not

 
 template <typename ValType>
 bool test(RunContext &Ctx, TestCondition Cond, ValType LHS, ValType RHS,
           const char *LHSStr, const char *RHSStr, const char *File,
           unsigned long Line);
 
 } // namespace internal
 
+struct MatcherBase {
+  virtual ~MatcherBase() {}
+  virtual void explainError(testutils::StreamWrapper &OS) {
+    OS << "unknown error\n";
+  }
+};
+
+template <typename T> struct Matcher : public MatcherBase { bool match(T &t); };
+
 // NOTE: One should not create instances and call methods on them directly. One
 // should use the macros TEST or TEST_F to write test cases.
 class Test {
 private:
   Test *Next = nullptr;
 
 public:
   virtual ~Test() {}
protected:
   static bool testStrEq(RunContext &Ctx, const char *LHS, const char *RHS,
                         const char *LHSStr, const char *RHSStr,
                         const char *File, unsigned long Line);
 
   static bool testStrNe(RunContext &Ctx, const char *LHS, const char *RHS,
                         const char *LHSStr, const char *RHSStr,
                         const char *File, unsigned long Line);
 
+  static bool testMatch(RunContext &Ctx, bool MatchResult, MatcherBase &Matcher,
+                        const char *LHSStr, const char *RHSStr,
+                        const char *File, unsigned long Line);
+
   static bool testProcessExits(RunContext &Ctx, testutils::FunctionCaller *Func,
                                int ExitCode, const char *LHSStr,
                                const char *RHSStr, const char *File,
                                unsigned long Line);
 
   static bool testProcessKilled(RunContext &Ctx,
                                 testutils::FunctionCaller *Func, int Signal,
                                 const char *LHSStr, const char *RHSStr,
#define EXPECT_DEATH(FUNC, SIG)                                                \
   __llvm_libc::testing::Test::testProcessKilled(                               \
       Ctx, __llvm_libc::testing::Test::createCallable(FUNC), SIG, #FUNC, #SIG, \
       __FILE__, __LINE__)
 
 #define ASSERT_DEATH(FUNC, EXIT)                                               \
   if (!EXPECT_DEATH(FUNC, EXIT))                                               \
   return
 
+#define __CAT1(a, b) a##b
+#define __CAT(a, b) __CAT1(a, b)
+#define UNIQUE_VAR(prefix) __CAT(prefix, __LINE__)
+
+#define EXPECT_THAT(MATCH, MATCHER)                                            \
+  auto UNIQUE_VAR(__matcher) = (MATCHER);                                      \
+  __llvm_libc::testing::Test::testMatch(                                       \
+      Ctx, UNIQUE_VAR(__matcher).match((MATCH)), UNIQUE_VAR(__matcher),        \
+      #MATCH, #MATCHER, __FILE__, __LINE__)
+
+#define ASSERT_THAT(MATCH, MATCHER)                                            \
+  if (!EXPECT_THAT(MATCH, MATCHER))                                            \
+  return
+
 #endif // LLVM_LIBC_UTILS_UNITTEST_H

libc/utils/UnitTest/Test.cpp

 
 bool Test::testStrNe(RunContext &Ctx, const char *LHS, const char *RHS,
                      const char *LHSStr, const char *RHSStr, const char *File,
                      unsigned long Line) {
   return internal::test(Ctx, Cond_NE, llvm::StringRef(LHS),
                         llvm::StringRef(RHS), LHSStr, RHSStr, File, Line);
 }
 
+bool Test::testMatch(RunContext &Ctx, bool MatchResult, MatcherBase &Matcher,
+                     const char *LHSStr, const char *RHSStr, const char *File,
+                     unsigned long Line) {
+  if (MatchResult)
+    return true;
+
+  Ctx.markFail();
+  llvm::outs() << File << ":" << Line << ": FAILURE\n"
+               << "Failed to match " << LHSStr << " against " << RHSStr
+               << ".\n";
+  testutils::StreamWrapper OutsWrapper = testutils::outs();
+  Matcher.explainError(OutsWrapper);
+  return false;
+}
+
 bool Test::testProcessKilled(RunContext &Ctx, testutils::FunctionCaller *Func,
                              int Signal, const char *LHSStr, const char *RHSStr,
                              const char *File, unsigned long Line) {
   testutils::ProcessStatus Result = testutils::invokeInSubprocess(Func);
 
   if (Result.exitedNormally()) {
     Ctx.markFail();
     llvm::outs() << File << ":" << Line << ": FAILURE\n"

libc/utils/testutils/CMakeLists.txt

 if(CMAKE_HOST_UNIX)
   set(EFFile ExecuteFunctionUnix.cpp)
 endif()
 
 add_llvm_library(
   libc_test_utils
+  StreamWrapper.cpp
+  StreamWrapper.h
   ${EFFile}
   ExecuteFunction.h
-  LINK_COMPONENTS Support
+  LINK_COMPONENTS
+    Support
 )

libc/utils/testutils/StreamWrapper.h

+//===------------------------ StreamWrapper.h -------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_UTILS_TESTUTILS_STREAMWRAPPER_H
+#define LLVM_LIBC_UTILS_TESTUTILS_STREAMWRAPPER_H
+
+namespace __llvm_libc {
+namespace testutils {
+
+// StreamWrapper is necessary because llvm/Support/raw_ostream.h includes
+// standard headers so we must provide streams through indirection to not
+// expose the system libc headers.
+class StreamWrapper {
+  void *OS;
+
+public:
+  StreamWrapper(void *OS) : OS(OS) {}
+
+  template <typename T> StreamWrapper &operator<<(T t);
+};
+
+StreamWrapper outs();
+
+} // namespace testutils
+} // namespace __llvm_libc
+
+#endif // LLVM_LIBC_UTILS_TESTUTILS_STREAMWRAPPER_H

libc/utils/testutils/StreamWrapper.cpp

+//===--------------------------- StreamWrapper.cpp ------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "StreamWrapper.h"
+#include "llvm/Support/raw_ostream.h"
+#include <cassert>
+#include <memory>
+
+namespace __llvm_libc {
+namespace testutils {
+
+StreamWrapper outs() { return {std::addressof(llvm::outs())}; }
+
+template <typename T> StreamWrapper &StreamWrapper::operator<<(T t) {
+  assert(OS);
+  llvm::raw_ostream &Stream = *reinterpret_cast<llvm::raw_ostream *>(OS);
+  Stream << t;
+  return *this;
+}
+
+template StreamWrapper &StreamWrapper::operator<<<const char *>(const char *t);
+template StreamWrapper &StreamWrapper::operator<<<char *>(char *t);
+template StreamWrapper &StreamWrapper::operator<<<char>(char t);
+template StreamWrapper &StreamWrapper::operator<<<short>(short t);
+template StreamWrapper &StreamWrapper::operator<<<int>(int t);
+template StreamWrapper &StreamWrapper::operator<<<long>(long t);
+template StreamWrapper &StreamWrapper::operator<<<long long>(long long t);
+template StreamWrapper &
+    StreamWrapper::operator<<<unsigned char>(unsigned char t);
+template StreamWrapper &
+    StreamWrapper::operator<<<unsigned short>(unsigned short t);
+template StreamWrapper &StreamWrapper::operator<<<unsigned int>(unsigned int t);
+template StreamWrapper &
+    StreamWrapper::operator<<<unsigned long>(unsigned long t);
+template StreamWrapper &
+    StreamWrapper::operator<<<unsigned long long>(unsigned long long t);
+template StreamWrapper &StreamWrapper::operator<<<bool>(bool t);
+
+} // namespace testutils
+} // namespace __llvm_libc