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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


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


================
Comment at: libc/utils/UnitTest/LibcTest.cpp:159
+      if (specific_binary != str_test_name) {
+        std::cout << YELLOW << "[ SKIPPING ] " << RESET << TestName << '\n';
+        break;
----------------
The skipping message can get really noisy, so may be just avoid it.


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105843