[libc] Add option to run specific tests

[caitlyncano]

Summary

This addition reads command line input to run specific single tests
within a larger call to run all the tests for a particular function.
When the user adds a second argument to the command line, the code skips
all the tests that don't match the user's specified binary. If the user
doesn't specify a test correctly and/or no tests are run, a failure
message prints.

[aeubanks]

could you run this through clang-format to properly format this? see the clang-format part of https://llvm.org/docs/Contributing.html#how-to-submit-a-patch

to be more specific about the commit title and the error message, we're not picking a specific _binary_, we're picking a specific _test_.

[caitlyncano]

[libc]

Fix formatting issues and adjust the no tests run message to be more specific.

[aeubanks]

variable names should match existing variable names and be CamelCase, not underscores

libc/utils/UnitTest/LibcTest.cpp

146

it's kinda weird to pass argc/argv directly to a function besides main. maybe just passing a std::optional<std::string> Filter is better

157

perhaps something like TestFilter?

158

IIRC std::string != const char* works, so probably no need to create str_test_name

160

this should be a continue, if we skip a test then we'll never run anything else after

(this is what writing a couple tests would help with)

182–183

I think just "No tests run." is good enough.

188

we should make this also fail if TestCount == 0

[sivachandra]

libc/utils/UnitTest/LibcTest.cpp

146

I agree - we should move the argv parsing code to a separate helper function. It would be cleaner and also scale (we plan to add ability to run flaky tests with run count etc.) @aeubanks' suggestion to use std::optional for the argument is perfect.

159

The skipping message can get really noisy, so may be just avoid it.

160

I agree here as well - we should write dummy tests to test this feature. It might require a little more surgery to the unit testing library. For example, we might have to separate the main function into a different library of its own like how gtest does.

182–183

+1. Also, you should add something like this:

if (TestFilter) {
  std::cout << "No mathing test for " << TestFilter << '\n';
  return 1;
}

The early return here will avoid checking for TestCount later.

[caitlyncano]

libc/utils/UnitTest/LibcTest.cpp

146

So the goal is to make a separate function to generate a Filter string that may/may not exist depending on the argc/argv conditions? I've never used the std::optional template so this is taking me a bit longer than expected to write out, many apologies!

[aeubanks]

libc/utils/UnitTest/LibcTest.cpp

146

yup

if we have argv[1] containing a string, set the std::optional<std::string> to std::optional<std::string>{argv[1]}
else set the std::optional<std::string> to std::nullopt

[caitlyncano]

libc/utils/UnitTest/LibcTest.cpp

146

Ok sounds great! It looks like std::optional needs another header, is adding #include <optional> in LibcTest.h okay or does libc have our own header somewhere?

[aeubanks]

libc/utils/UnitTest/LibcTest.cpp

146

That's a C++ (not a C) header, so #include <optional> is fine

[sivachandra]

libc/utils/UnitTest/LibcTest.cpp

146

Sigh! Even C++ headers cannot be included as they can potentially include other C headers and lead to mixups. We have a standalone, C++ utilities library but it does not have std::optional: https://github.com/llvm/llvm-project/tree/main/libc/utils/CPP

One not so C++ like option for the interim is to use const char * and condition on whether it is a nullptr or not.

[caitlyncano]

[libc] Simplify test name comparisons

This adjustment resolves some format/linting issues, and simplifies some of the
logic used to compare the user-inputted test filter and the iterated test name.
It creates a new function to set the test filter as a string outside of main
to avoid passing the command line variables directly to runTests.

[aeubanks]

this is looking much better

I think you need to clang-format again

libc/utils/UnitTest/LibcTest.cpp

183

"No tests run.\n"

186

I'm tempted to always fail if we didn't run any tests since that seems like it should never happen, any thoughts?

361

this is more of a get than a set

361

I think keeping argc/argv as the variable names is clearer, that way it's obvious these are directly accessing the command line flags

[caitlyncano]

[libc] Fail if no tests run

This addition adjusts the final conditionals to return 1 to indicate failure,
and fixes formatting errors.

[sivachandra]

Almost there.

libc/utils/UnitTest/LibcTest.cpp

184

Please run clang-format for all patches touching c++ code. It will fix errors like this. Also, LLVM style says we shouldn't be using the curly brackets for single statement blocks unless the statement spans multiple lines. So, this if block does not need the curly brackets.

186

Either way is fine.

[caitlyncano]

libc/utils/UnitTest/LibcTest.cpp

184

I ran clang format before this last upload, but it only caught some of the things so I'm just going to hand-fix the leftovers!

[aeubanks]

git clang-format HEAD~ -f should work (if you're using git-clang-format)

[sivachandra]

Sorry, another small nit.

libc/utils/UnitTest/LibcTest.cpp

361

Can this function be static like this and avoid the public declaration as pointed out above:

static const char *getTestFileter(...) {
  ...
}

libc/utils/UnitTest/LibcTest.h

59

Do we need this declaration?

[caitlyncano]

[libc] Fix formatting errors

[caitlyncano]

[libc] Redeclare getTestFilter as static

[aeubanks]

the code looks good, but it'd be nice if we had tests for this

libc/utils/UnitTest/LibcTest.cpp

179

I think it'd be nice if we listed the number of skipped tests, or just the fact that there is a filter at all. But not necessary right now.

364–365

merge these two into one line

[caitlyncano]

[libc] Remove Main from LibcTest.cpp

This patch takes the main function out of LibcTest.cpp and moves it to its own
file, in addition to adding infrastructure to support testing of tests by
allowing opt-in inclusion of that main or to use the main within
testfilter_test.cpp to test the runTests functionality.

[caitlyncano]

[libc] Add UnitTests directory in libc/utils

[caitlyncano]

[libc] Update previous patch edit for functional linking

[caitlyncano]

[libc] Add testfilter tests

[aeubanks]

libc/test/utils/CMakeLists.txt

10

extraneous empty lines

libc/test/utils/UnitTest/testfilter_test.cpp

19

what are these for?

21

IncorrectFilter

33

I'd add a test that tests that we run a test that's not the first test in the file, since that was one of the bugs that came up earlier.

e.g. add a CorrectFilter2 and run that via a filter

38

it's weird to me that we're using runTests() to test runTests(), but hey I guess if it works...

libc/utils/UnitTest/LibcTest.cpp

2

this shouldn't change

186

moving this to be part of the return statement below would be clearer, it's confusing when we have multiple unbalanced return statements

something like return FailCount > 0 || TestCount == 0 ? 1 : 0;

[caitlyncano]

[libc] Add additional check for correct results and fix formatting

[aeubanks]

libc/test/utils/UnitTest/testfilter_test.cpp

12–15

is all of this is no longer necessary?

libc/utils/UnitTest/LibcTest.cpp

1

unintentional change?

libc/utils/UnitTest/LibcTestMain.cpp

3

formatting

13–16

ditto

[caitlyncano]

libc/utils/UnitTest/LibcTestMain.cpp

13–16

Just need to keep <string> to return NULL, but got rid of the unnecessary ones. Thanks!!

[caitlyncano]

[libc] Remove unnecessary headers and fix formatting

[aeubanks]

libc/test/utils/UnitTest/testfilter_test.cpp

12

this still isn't used right?

libc/utils/UnitTest/LibcTestMain.cpp

13–16

it seemed weird to me that <string> defined NULL, so I did a little research and apparently <cstddef> is actually what defines it, <string> probably just includes <cstddef>

but in any case, in C++ we should use nullptr over NULL, and we won't need to include either

[sivachandra]

libc/cmake/modules/LLVMLibCTestRules.cmake

77

May be name is it NO_LIBC_UNITTEST_TEST_MAIN.

153

You should not add testfileter_test here. It will be its own binary.

libc/test/utils/UnitTest/CMakeLists.txt

2

The suite name should probably be libc_unittest_tests.

libc/utils/UnitTest/CMakeLists.txt

16

Can you actually add a target as an include directory?

libc/utils/UnitTest/LibcTestMain.cpp

12

Is this used in this file?

[caitlyncano]

libc/utils/UnitTest/CMakeLists.txt

16

It didn't cause any issues to take LibcUnitTest out and the documentation doesn't warn about any problems with it, so I'm just going to leave it out. Does ${LIBC_SOURCE_DIR} include it after line 7 is run?

[caitlyncano]

[libc] Fix variable names and remove unnecessary lines

[aeubanks]

super nit: the very first lines of the files that start with //=== are inconsistent, I think usually it's //===-- description here ------===//
otherwise lgtm

[sivachandra]

In D105843#2898229, @aeubanks wrote:

super nit: the very first lines of the files that start with //=== are inconsistent, I think usually it's //===-- description here ------===//
otherwise lgtm

+1

[caitlyncano]

[libc] Fix title formatting

[caitlyncano]

[libc] Remove accidental edits to windows config

[gchatelet]

libc/utils/UnitTest/CMakeLists.txt

18

This line should be target_link_libraries(LibcUnitTestMain PUBLIC **LibcUnitTest** libc_test_utils) otherwise build is failing when building with shared libraries.
Fixed in https://reviews.llvm.org/rG47afd43eaa9b

libc/cmake/modules/LLVMLibCTestRules.cmake

if(NOT LLVM_INCLUDE_TESTS)
     return()
   endif()
 
   cmake_parse_arguments(
     "LIBC_UNITTEST"
     "" # No optional arguments
     "SUITE" # Single value arguments
     "SRCS;HDRS;DEPENDS;COMPILE_OPTIONS" # Multi-value arguments
+    "NO_LIBC_UNITTEST_TEST_MAIN"
     ${ARGN}
   )
   if(NOT LIBC_UNITTEST_SRCS)
     message(FATAL_ERROR "'add_libc_unittest' target requires a SRCS list of .cpp "
                         "files.")
   endif()
   if(NOT LIBC_UNITTEST_DEPENDS)
     message(FATAL_ERROR "'add_libc_unittest' target requires a DEPENDS list of "
function(add_libc_unittest target_name)
   set_target_properties(${fq_target_name}
     PROPERTIES RUNTIME_OUTPUT_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR})
 
   add_dependencies(
     ${fq_target_name}
     ${fq_deps_list}
   )
 
+  if(NO_LIBC_UNITTEST_TEST_MAIN)
     target_link_libraries(${fq_target_name} PRIVATE LibcUnitTest libc_test_utils)
+  else()
+    target_link_libraries(${fq_target_name} PRIVATE LibcUnitTest LibcUnitTestMain libc_test_utils)
+  endif()
 
   add_custom_command(
     TARGET ${fq_target_name}
     POST_BUILD
     COMMAND $<TARGET_FILE:${fq_target_name}>
   )
   if(LIBC_UNITTEST_SUITE)
     add_dependencies(

libc/test/utils/CMakeLists.txt

 add_subdirectory(FPUtil)
 add_subdirectory(CPP)
+add_subdirectory(UnitTest)
 
 if(NOT LLVM_LIBC_FULL_BUILD)
   return()
 endif()
 
 add_subdirectory(tools)

libc/test/utils/UnitTest/CMakeLists.txt

+add_libc_testsuite(libc_unittest_tests)
+
+add_libc_unittest(
+  testfilter_test
+  SUITE
+    libc_unittest_tests
+  SRCS
+    testfilter_test.cpp
+  DEPENDS
+    libc.utils.CPP.standalone_cpp
+)

libc/test/utils/UnitTest/testfilter_test.cpp

+//===-- Tests for Test Filter functionality -------------------------------===//
+//
+// 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 "utils/UnitTest/LibcTest.h"
+
+TEST(LlvmLibcTestFilterTest, CorrectFilter) {}
+
+TEST(LlvmLibcTestFilterTest, CorrectFilter2) {}
+
+TEST(LlvmLibcTestFilterTest, IncorrectFilter) {}
+
+TEST(LlvmLibcTestFilterTest, NoFilter) {}
+
+TEST(LlvmLibcTestFilterTest, CheckCorrectFilter) {
+  ASSERT_EQ(
+      __llvm_libc::testing::Test::runTests("LlvmLibcTestFilterTest.NoFilter"),
+      0);
+  ASSERT_EQ(__llvm_libc::testing::Test::runTests(
+                "LlvmLibcTestFilterTest.IncorrFilter"),
+            1);
+  ASSERT_EQ(__llvm_libc::testing::Test::runTests(
+                "LlvmLibcTestFilterTest.CorrectFilter"),
+            0);
+  ASSERT_EQ(__llvm_libc::testing::Test::runTests(
+                "LlvmLibcTestFilterTest.CorrectFilter2"),
+            0);
+}
+
+int main() {
+  __llvm_libc::testing::Test::runTests(
+      "LlvmLibcTestFilterTest.CheckCorrectFilter");
+  return 0;
+}

libc/utils/UnitTest/CMakeLists.txt

 add_library(
   LibcUnitTest
   Test.h
   LibcTest.cpp
   LibcTest.h
 )
 target_include_directories(LibcUnitTest PUBLIC ${LIBC_SOURCE_DIR})
 add_dependencies(LibcUnitTest libc.utils.CPP.standalone_cpp)
 target_link_libraries(LibcUnitTest PUBLIC libc_test_utils)
+
+add_library(
+  LibcUnitTestMain
+  LibcTestMain.cpp
+)
+
+target_include_directories(LibcUnitTestMain PUBLIC ${LIBC_SOURCE_DIR})
+add_dependencies(LibcUnitTestMain LibcUnitTest libc.utils.CPP.standalone_cpp)
+target_link_libraries(LibcUnitTestMain PUBLIC libc_test_utils)
+

libc/utils/UnitTest/LibcTest.h

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;
   RunContext *Ctx = nullptr;
 
   void setContext(RunContext *C) { Ctx = C; }
 
 public:
   virtual ~Test() {}
   virtual void SetUp() {}
   virtual void TearDown() {}
 
-  static int runTests();
+  static int runTests(const char *);
 
 protected:
   static void addTest(Test *T);
 
   // We make use of a template function, with |LHS| and |RHS| as explicit
   // parameters, for enhanced type checking. Other gtest like unittest
   // frameworks have a similar function which takes a boolean argument
   // instead of the explicit |LHS| and |RHS| arguments. This boolean argument

libc/utils/UnitTest/LibcTest.cpp

-//===-- Implementation of the base class for libc unittests ---------------===//
+//===-- Implementation of the base class for libc unittests----------------===//
 //
 // 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 "LibcTest.h"
 
if (End == nullptr) {
     End = T;
     return;
   }
 
   End->Next = T;
   End = T;
 }
 
-int Test::runTests() {
+int Test::runTests(const char *TestFilter) {
   int TestCount = 0;
   int FailCount = 0;
-  for (Test *T = Start; T != nullptr; T = T->Next, ++TestCount) {
+  for (Test *T = Start; T != nullptr; T = T->Next) {
     const char *TestName = T->getName();
+    std::string StrTestName(TestName);
     constexpr auto GREEN = "\033[32m";
     constexpr auto RED = "\033[31m";
     constexpr auto RESET = "\033[0m";
+    if ((TestFilter != nullptr) && (StrTestName != TestFilter)) {
+      continue;
+    }
     std::cout << GREEN << "[ RUN      ] " << RESET << TestName << '\n';
     RunContext Ctx;
     T->SetUp();
     T->setContext(&Ctx);
     T->Run();
     T->TearDown();
     auto Result = Ctx.status();
     switch (Result) {
     case RunContext::Result_Fail:
       std::cout << RED << "[  FAILED  ] " << RESET << TestName << '\n';
       ++FailCount;
       break;
     case RunContext::Result_Pass:
       std::cout << GREEN << "[       OK ] " << RESET << TestName << '\n';
       break;
     }
+    ++TestCount;
   }
 
+  if (TestCount > 0) {
     std::cout << "Ran " << TestCount << " tests. "
               << " PASS: " << TestCount - FailCount << ' '
               << " FAIL: " << FailCount << '\n';
+  } else {
+    std::cout << "No tests run.\n";
+    if (TestFilter) {
+      std::cout << "No matching test for " << TestFilter << '\n';
+    }
+  }
 
-  return FailCount > 0 ? 1 : 0;
+  return FailCount > 0 || TestCount == 0 ? 1 : 0;
 }
 
 template bool Test::test<char, 0>(TestCondition Cond, char LHS, char RHS,
                                   const char *LHSStr, const char *RHSStr,
                                   const char *File, unsigned long Line);
 
 template bool Test::test<short, 0>(TestCondition Cond, short LHS, short RHS,
                                    const char *LHSStr, const char *RHSStr,
bool Test::testProcessExits(testutils::FunctionCaller *Func, int ExitCode,
   std::cout << File << ":" << Line << ": FAILURE\n"
             << "Expected exit code of: " << LHSStr << '\n'
             << "             Which is: " << ActualExit << '\n'
             << "       To be equal to: " << RHSStr << '\n'
             << "             Which is: " << ExitCode << '\n';
   return false;
 }
 
 #endif // ENABLE_SUBPROCESS_TESTS
 } // namespace testing
 } // namespace __llvm_libc
-
-int main() { return __llvm_libc::testing::Test::runTests(); }

libc/utils/UnitTest/LibcTestMain.cpp

+//===-- Main function for implementation of base class for libc unittests -===//
+//
+// 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 "LibcTest.h"
+
+static const char *getTestFilter(int argc, char *argv[]) {
+  return argc > 1 ? argv[1] : nullptr;
+}
+
+int main(int argc, char *argv[]) {
+  const char *TestFilter = getTestFilter(argc, argv);
+  return __llvm_libc::testing::Test::runTests(TestFilter);
+}