[libc-commits] [PATCH] D105843: [libc] Add option to run specific tests

[Arthur Eubanks via Phabricator via libc-commits]

aeubanks added a comment.

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



================
Comment at: libc/utils/UnitTest/LibcTest.cpp:146
 
-int Test::runTests() {
+int Test::runTests(int argc, char *argv[]) {
   int TestCount = 0;
----------------
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


================
Comment at: libc/utils/UnitTest/LibcTest.cpp:157
+    if (argc > 1) {
+      std::string specific_binary(argv[1]);
+      if (specific_binary != str_test_name) {
----------------
perhaps something like `TestFilter`?


================
Comment at: libc/utils/UnitTest/LibcTest.cpp:158
+      std::string specific_binary(argv[1]);
+      if (specific_binary != str_test_name) {
+        std::cout << YELLOW << "[ SKIPPING ] " << RESET << TestName << '\n';
----------------
IIRC `std::string != const char*` works, so probably no need to create `str_test_name`


================
Comment at: libc/utils/UnitTest/LibcTest.cpp:160
+        std::cout << YELLOW << "[ SKIPPING ] " << RESET << TestName << '\n';
+        break;
+      }
----------------
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)


================
Comment at: libc/utils/UnitTest/LibcTest.cpp:186-187
+  } else {
+    std::cout << "No tests run. Ensure that inputted test matches the "
+                 "test name exactly.";
+  }
----------------
I think just "No tests run." is good enough.


================
Comment at: libc/utils/UnitTest/LibcTest.cpp:190
 
   return FailCount > 0 ? 1 : 0;
 }
----------------
we should make this also fail if `TestCount == 0`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105843/new/

https://reviews.llvm.org/D105843