[libc-commits] [PATCH] D92137: [libc] add tests to WrapperGen

[Alex Brachet via Phabricator via libc-commits]

abrachet added a comment.

In D92137#2423446 <https://reviews.llvm.org/D92137#2423446>, @michaelrj wrote:

> I'm not quite sure why a lit test would be better in this situation, does it have better support for testing complete binaries?

Yes. Here's an example test with FileCheck https://github.com/llvm/llvm-project/blob/master/llvm/test/tools/llvm-nm/invalid-input.test

> `// Currently it's just comparing the output of the program to an exact string, this means that even a small formatting change would break this test.`

This is what FileCheck was designed for



================
Comment at: libc/test/utils/tools/WrapperGen/wrappergen_test.cpp:36-39
+  llvm::Expected<llvm::sys::fs::TempFile> STDOutFile =
+      llvm::sys::fs::TempFile::create("wrappergen-stdout-%%-%%-%%-%%.txt");
+  llvm::Expected<llvm::sys::fs::TempFile> STDErrFile =
+      llvm::sys::fs::TempFile::create("wrappergen-stderr-%%-%%-%%-%%.txt");
----------------
These should be constructed in `SetUp`. I'm not sure the behavior is defined if they are discarded in `TearDown` then later reused after `TearDown`


================
Comment at: libc/test/utils/tools/WrapperGen/wrappergen_test.cpp:51-56
+    if (!STDOutFile) {
+      llvm::errs() << "Error: " << llvm::toString(STDOutFile.takeError())
+                   << "\n";
+      llvm::report_fatal_error(
+          "Temporary file failed to initialize for libc-wrappergen tests.");
+    }
----------------
Maybe
```lang=CPP
llvm::ExitOnError errCheck("Temporary file failed to initialize for libc-wrappergen tests.");
errCheck(STDOutFile);
errCheck(STDErrFile);
```


================
Comment at: libc/test/utils/tools/WrapperGen/wrappergen_test.cpp:83-90
+  constexpr size_t BufferSize = 128;
+  char Buffer[BufferSize];
+
+  int STDErrFD = STDErrFile.get().FD;
+  std::string STDErrOutput;
+  while (read(STDErrFD, Buffer, BufferSize)) {
+    STDErrOutput.append(Buffer);
----------------
`std::string::append(const char *)` will expect that the string it is null terminated. `::read` can't do that.

https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/Support/MemoryBuffer.h#L86
`llvm::MemoryBuffer` may be easier to use.


================
Comment at: libc/test/utils/tools/WrapperGen/wrappergen_test.cpp:121
+  while (read(STDOutFD, Buffer, BufferSize)) {
+
+    STDOutOutput.append(Buffer);
----------------
Not needed. Nor are the brackets on these 1 line while statements


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92137