[RFC][libc] shut up passing tests #117253

nickdesaulniers · 2024-11-21T22:43:08Z

When a test fails, you have to scroll quite a bit to find which test failed.
This gets worse over time as we add more tests.

gtest has the option --gtest_brief=1 to silence passing tests from the output.
Add this to our pseudo-gtest framework.

With this change, only failing tests are printed. Example:

$ ninja libc-unit-tests
[1206/1357] Running unit test libc.test.src.string.memchr_test.unit
FAILED: libc/test/src/string/CMakeFiles/libc.test.src.string.memchr_test.unit /android0/llvm-project/build/libc/test/src/string/CMakeFiles/libc.test.src.string.memchr_test.unit
cd /android0/llvm-project/build/libc/test/src/string && /android0/llvm-project/build/libc/test/src/string/libc.test.src.string.memchr_test.unit.build --gtest_brief=${BRIEF:-1}
/android0/llvm-project/libc/test/src/string/memchr_test.cpp:24: FAILURE
Expected: call_memchr(src, 'b', size)
Which is: bc
To be equal to: "b"
Which is: b
[ FAILED ] LlvmLibcMemChrTest.FindsCharacterAfterNullTerminator

There are currently two tests that do print output which can probably also get
cleaned up (they are kind of like expected failures):

stack_chk_guard_test
LlvmLibcTestFilterTest.IncorrFilter

To get the old behavior, one can do:

$ BRIEF=0 ninja libc-unit-tests

Longer term, I'd like to move to use of llvm-lit, as is used throughout most of
the rest of llvm.

We can't make this the default behavior, since Android's atest framework needs the
default behavior of printing all tests run. Instead, this PR adds a flag that is passed
by our cmake.

Link: https://google.github.io/googletest/advanced.html#suppressing-test-passes

llvmbot · 2024-11-21T22:43:47Z

@llvm/pr-subscribers-libc

Author: Nick Desaulniers (nickdesaulniers)

Changes

When a test fails, you have to scroll quite a bit to find which test failed.
This gets worst over time as we add more tests.

gtest has the option --gtest_brief=1 to silence passing tests from the output.
Add this to our pseudo-gtest framework.

With this change, only failing tests are printed. Example:

$ ninja libc-unit-tests
[1206/1357] Running unit test libc.test.src.string.memchr_test.unit
FAILED: libc/test/src/string/CMakeFiles/libc.test.src.string.memchr_test.unit /android0/llvm-project/build/libc/test/src/string/CMakeFiles/libc.test.src.string.memchr_test.unit
cd /android0/llvm-project/build/libc/test/src/string && /android0/llvm-project/build/libc/test/src/string/libc.test.src.string.memchr_test.unit.build --gtest_brief=${BRIEF:-1}
/android0/llvm-project/libc/test/src/string/memchr_test.cpp:24: FAILURE
Expected: call_memchr(src, 'b', size)
Which is: bc
To be equal to: "b"
Which is: b
[ FAILED ] LlvmLibcMemChrTest.FindsCharacterAfterNullTerminator

There are currently two tests that do print output which can probably also get
cleaned up (they are kind of like expected failures):

stack_chk_guard_test
LlvmLibcTestFilterTest.IncorrFilter

To get the old behavior, one can do:

$ BRIEF=0 ninja libc-unit-tests

Longer term, I'd like to move to use of llvm-lit, as is used throughout most of
the rest of llvm.

Link: https://google.github.io/googletest/advanced.html#suppressing-test-passes

Full diff: https://github.com/llvm/llvm-project/pull/117253.diff

6 Files Affected:

(modified) libc/cmake/modules/LLVMLibCTestRules.cmake (+3-3)
(modified) libc/test/UnitTest/LibcTest.cpp (+14-6)
(modified) libc/test/UnitTest/LibcTest.h (+2)
(modified) libc/test/UnitTest/LibcTestMain.cpp (+2)
(modified) libc/test/src/string/memchr_test.cpp (+1-1)
(modified) libc/test/utils/UnitTest/testfilter_test.cpp (+3)

diff --git a/libc/cmake/modules/LLVMLibCTestRules.cmake b/libc/cmake/modules/LLVMLibCTestRules.cmake
index c3a0f371cd6201..3435713c114cde 100644
--- a/libc/cmake/modules/LLVMLibCTestRules.cmake
+++ b/libc/cmake/modules/LLVMLibCTestRules.cmake
@@ -235,7 +235,7 @@ function(create_libc_unittest fq_target_name)
   if(NOT LIBC_UNITTEST_NO_RUN_POSTBUILD)
     add_custom_target(
       ${fq_target_name}
-      COMMAND ${fq_build_target_name}
+      COMMAND ${fq_build_target_name} "--gtest_brief=\$\${BRIEF:-1}"
       COMMENT "Running unit test ${fq_target_name}"
     )
   endif()
@@ -526,7 +526,7 @@ function(add_integration_test test_name)
       $<TARGET_FILE:${fq_build_target_name}> ${INTEGRATION_TEST_ARGS})
   add_custom_target(
     ${fq_target_name}
-    COMMAND ${test_cmd}
+    COMMAND ${test_cmd} "--gtest_brief=\$\${BRIEF:-1}"
     COMMAND_EXPAND_LISTS
     COMMENT "Running integration test ${fq_target_name}"
   )
@@ -719,7 +719,7 @@ function(add_libc_hermetic test_name)
 
   add_custom_command(
     OUTPUT ${fq_target_name}-cmd
-    COMMAND ${test_cmd}
+    COMMAND ${test_cmd} "--gtest_brief=\$\${BRIEF:-1}"
     COMMAND_EXPAND_LISTS
     COMMENT "Running hermetic test ${fq_target_name}"
     ${LIBC_HERMETIC_TEST_JOB_POOL}
diff --git a/libc/test/UnitTest/LibcTest.cpp b/libc/test/UnitTest/LibcTest.cpp
index afb1368f009050..955b7ba1dc2c79 100644
--- a/libc/test/UnitTest/LibcTest.cpp
+++ b/libc/test/UnitTest/LibcTest.cpp
@@ -140,7 +140,7 @@ int Test::runTests(const TestOptions &Options) {
   const char *reset = Options.PrintColor ? "\033[0m" : "";
 
   int TestCount = getNumTests();
-  if (TestCount) {
+  if (TestCount && !Options.Brief) {
     tlog << green << "[==========] " << reset << "Running " << TestCount
          << " test";
     if (TestCount > 1)
@@ -157,7 +157,8 @@ int Test::runTests(const TestOptions &Options) {
       continue;
     }
 
-    tlog << green << "[ RUN      ] " << reset << TestName << '\n';
+    if (!Options.Brief)
+      tlog << green << "[ RUN      ] " << reset << TestName << '\n';
     [[maybe_unused]] const uint64_t start_time = clock();
     RunContext Ctx;
     T->SetUp();
@@ -171,7 +172,12 @@ int Test::runTests(const TestOptions &Options) {
       ++FailCount;
       break;
     case RunContext::RunResult::Pass:
-      tlog << green << "[       OK ] " << reset << TestName;
+      if (!Options.Brief)
+        tlog << green << "[       OK ] " << reset << TestName;
+
+      if (Options.Brief)
+        break;
+
 #ifdef LIBC_TEST_USE_CLOCK
       tlog << " (";
       if (start_time > end_time) {
@@ -197,9 +203,11 @@ int Test::runTests(const TestOptions &Options) {
   }
 
   if (TestCount > 0) {
-    tlog << "Ran " << TestCount << " tests. "
-         << " PASS: " << TestCount - FailCount << ' ' << " FAIL: " << FailCount
-         << '\n';
+    if (!Options.Brief) {
+      tlog << "Ran " << TestCount << " tests. "
+           << " PASS: " << TestCount - FailCount << ' ' << " FAIL: " << FailCount
+           << '\n';
+    }
   } else {
     tlog << "No tests run.\n";
     if (Options.TestFilter) {
diff --git a/libc/test/UnitTest/LibcTest.h b/libc/test/UnitTest/LibcTest.h
index b4e3819ea958de..a175a4615f6f81 100644
--- a/libc/test/UnitTest/LibcTest.h
+++ b/libc/test/UnitTest/LibcTest.h
@@ -108,6 +108,8 @@ struct TestOptions {
   bool PrintColor = true;
   // Should the test results print timing only in milliseconds, as GTest does?
   bool TimeInMs = false;
+  // Should passing tests be suppressed?
+  bool Brief = false;
 };
 
 // NOTE: One should not create instances and call methods on them directly. One
diff --git a/libc/test/UnitTest/LibcTestMain.cpp b/libc/test/UnitTest/LibcTestMain.cpp
index c348d5ef1aa1b1..3ed728d70fca68 100644
--- a/libc/test/UnitTest/LibcTestMain.cpp
+++ b/libc/test/UnitTest/LibcTestMain.cpp
@@ -31,6 +31,8 @@ TestOptions parseOptions(int argc, char **argv) {
       Options.PrintColor = false;
     else if (arg == "--gtest_print_time")
       Options.TimeInMs = true;
+    else if (arg == "--gtest_brief=1")
+      Options.Brief = true;
     // Ignore other unsupported gtest specific flags.
     else if (arg.starts_with("--gtest_"))
       continue;
diff --git a/libc/test/src/string/memchr_test.cpp b/libc/test/src/string/memchr_test.cpp
index 343958234edcee..efd3e55421c4be 100644
--- a/libc/test/src/string/memchr_test.cpp
+++ b/libc/test/src/string/memchr_test.cpp
@@ -21,7 +21,7 @@ TEST(LlvmLibcMemChrTest, FindsCharacterAfterNullTerminator) {
   const size_t size = 5;
   const unsigned char src[size] = {'a', '\0', 'b', 'c', '\0'};
   // Should return 'b', 'c', '\0' even when after null terminator.
-  ASSERT_STREQ(call_memchr(src, 'b', size), "bc");
+  ASSERT_STREQ(call_memchr(src, 'b', size), "b");
 }
 
 TEST(LlvmLibcMemChrTest, FindsCharacterInNonNullTerminatedCollection) {
diff --git a/libc/test/utils/UnitTest/testfilter_test.cpp b/libc/test/utils/UnitTest/testfilter_test.cpp
index d7e68252cf2aae..d3513dc4169bcf 100644
--- a/libc/test/utils/UnitTest/testfilter_test.cpp
+++ b/libc/test/utils/UnitTest/testfilter_test.cpp
@@ -20,6 +20,9 @@ TEST(LlvmLibcTestFilterTest, NoFilter) {}
 
 TEST(LlvmLibcTestFilterTest, CheckCorrectFilter) {
   TestOptions Options;
+
+  Options.Brief = true;
+
   Options.TestFilter = "LlvmLibcTestFilterTest.NoFilter";
   ASSERT_EQ(LIBC_NAMESPACE::testing::Test::runTests(Options), 0);

nickdesaulniers · 2024-11-21T22:44:39Z

libc/test/UnitTest/LibcTestMain.cpp

@@ -31,6 +31,8 @@ TestOptions parseOptions(int argc, char **argv) {
      Options.PrintColor = false;
    else if (arg == "--gtest_print_time")
      Options.TimeInMs = true;
+    else if (arg == "--gtest_brief=1")
+      Options.Brief = true;


Note: gtest also support the GTEST_BRIEF env var. To make this cleaner in the Cmake files, perhaps I should only support the env var, and not the command line flag?

libc/test/UnitTest/LibcTest.cpp

github-actions · 2024-11-21T22:46:42Z

✅ With the latest revision this PR passed the C/C++ code formatter.

libc/test/src/string/memchr_test.cpp

When a test fails, you have to scroll quite a bit to find which test failed. This gets worst over time as we add more tests. gtest has the option --gtest_brief=1 to silence passing tests from the output. Add this to our pseudo-gtest framework. With this change, only failing tests are printed. Example: $ ninja libc-unit-tests [1206/1357] Running unit test libc.test.src.string.memchr_test.__unit__ FAILED: libc/test/src/string/CMakeFiles/libc.test.src.string.memchr_test.__unit__ /android0/llvm-project/build/libc/test/src/string/CMakeFiles/libc.test.src.string.memchr_test.__unit__ cd /android0/llvm-project/build/libc/test/src/string && /android0/llvm-project/build/libc/test/src/string/libc.test.src.string.memchr_test.__unit__.__build__ --gtest_brief=${BRIEF:-1} /android0/llvm-project/libc/test/src/string/memchr_test.cpp:24: FAILURE Expected: call_memchr(src, 'b', size) Which is: bc To be equal to: "b" Which is: b [ FAILED ] LlvmLibcMemChrTest.FindsCharacterAfterNullTerminator There are currently two tests that do print output which can probably also get cleaned up (they are kind of like expected failures): - stack_chk_guard_test - LlvmLibcTestFilterTest.IncorrFilter To get the old behavior, one can do: $ BRIEF=0 ninja libc-unit-tests Longer term, I'd like to move to use of llvm-lit, as is used throughout most of the rest of llvm. Link: https://google.github.io/googletest/advanced.html#suppressing-test-passes

jhuber6

Honestly I think the real solution is to use LLVM's lit.

nickdesaulniers · 2024-12-04T17:09:19Z

Sure; let me familiarize myself with libc++ and compiler-rt's testing infra. I'm very familiar with llvm's lit and unit testing, but I don't know what the runtimes do across the board.

nickdesaulniers · 2024-12-04T17:11:18Z

https://libcxx.llvm.org/TestingLibcxx.html#usage as an example.

jhuber6 · 2024-12-04T17:14:48Z

https://libcxx.llvm.org/TestingLibcxx.html#usage as an example.

I remember bringing this up way back, at the time the assertions was the external users wanted to use it so having a depedency on lit would be annoying. But I think we're the only test suite that doesn't use lit primarily, (Other tests have unit tests which is basically what all of ours are, but they're still handed through lit).

The desired interface (for me) would be something like

> ninja check-libc-amdgcn-amd-amdhsa
# Runs all tests
> ./bin/llvm-lit runttimes/runtimes-amdgcn-amd-amdhsa-bins/libc/test
# Also runs all tests
> ./bin/llvm-lit runttimes/runtimes-amdgcn-amd-amdhsa-bins/libc/test/src/stdio
# Runs all stdio tests

nickdesaulniers · 2024-12-04T17:47:45Z

Yeah, I agree 💯 %. Was trying to see if we had a bug on file for moving to llvm-lit ...

nickdesaulniers · 2024-12-04T20:34:55Z

Will close this in favor of #118694.

llvmbot added the libc label Nov 21, 2024

nickdesaulniers requested review from SchrodingerZhu, jhuber6, lntue and michaelrj-google November 21, 2024 22:43

nickdesaulniers commented Nov 21, 2024

View reviewed changes

libc/test/UnitTest/LibcTest.cpp Show resolved Hide resolved

lntue reviewed Nov 21, 2024

View reviewed changes

libc/test/src/string/memchr_test.cpp Outdated Show resolved Hide resolved

nickdesaulniers force-pushed the gtest_brief branch from e324310 to af5b3bc Compare December 4, 2024 17:02

jhuber6 reviewed Dec 4, 2024

View reviewed changes

nickdesaulniers mentioned this pull request Dec 4, 2024

[libc] support llvm-lit as a test runner #118694

Open

nickdesaulniers closed this Dec 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC][libc] shut up passing tests #117253

[RFC][libc] shut up passing tests #117253

nickdesaulniers commented Nov 21, 2024 •

edited

Loading

llvmbot commented Nov 21, 2024

nickdesaulniers Nov 21, 2024

github-actions bot commented Nov 21, 2024 •

edited

Loading

jhuber6 left a comment

nickdesaulniers commented Dec 4, 2024

nickdesaulniers commented Dec 4, 2024

jhuber6 commented Dec 4, 2024

nickdesaulniers commented Dec 4, 2024

nickdesaulniers commented Dec 4, 2024

[RFC][libc] shut up passing tests #117253

[RFC][libc] shut up passing tests #117253

Conversation

nickdesaulniers commented Nov 21, 2024 • edited Loading

llvmbot commented Nov 21, 2024

nickdesaulniers Nov 21, 2024

Choose a reason for hiding this comment

github-actions bot commented Nov 21, 2024 • edited Loading

jhuber6 left a comment

Choose a reason for hiding this comment

nickdesaulniers commented Dec 4, 2024

nickdesaulniers commented Dec 4, 2024

jhuber6 commented Dec 4, 2024

nickdesaulniers commented Dec 4, 2024

nickdesaulniers commented Dec 4, 2024

nickdesaulniers commented Nov 21, 2024 •

edited

Loading

github-actions bot commented Nov 21, 2024 •

edited

Loading