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

[Michael Jones via Phabricator via libc-commits]

michaelrj added a comment.

after talking with Siva about it it looks like we're going to stick with this for the moment and not go with lit tests. The reason is that currently the wrappergen unit tests about as dependent on LLVM support as wrappergen itself, adding lit tests would make portability potentially harder.



================
Comment at: libc/test/utils/tools/WrapperGen/CMakeLists.txt:6
+    wrappergen_test.cpp   
+  DEPENDS
+  ARGS
----------------
sivachandra wrote:
> Why list an empty `DEPENDS`?
it was a leftover from when I was trying adding libc-wrappergen as a full dependency.


================
Comment at: libc/test/utils/tools/WrapperGen/wrappergen_test.cpp:27
+             llvm::cl::value_desc("<path to tool>"), llvm::cl::Required);
+llvm::cl::opt<std::string>
+    TargetOS("targetos", llvm::cl::desc("Name of the target operating system."),
----------------
sivachandra wrote:
> We should make this test independent of the target OS/machine. That is why I suggested using a test API file. Did you run into any problems with that approach?
no problems, I just hadn't done it yet. It's now done and at `test/utils/tools/WrapperGen/testapi.td`


================
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");
----------------
abrachet wrote:
> 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`
Since I'm using test fixtures a new one is defined for each TEST_F (see documentation copied below) so each file path is actually different (I even tested this and got the following for one run with both existing tests: 
wrappergen-stdout-5b-5b-2e-bb.txt
wrappergen-stderr-e2-c7-cb-19.txt
wrappergen-stdout-19-07-f4-2b.txt
wrappergen-stderr-db-35-0f-06.txt).
The reason I'm not initializing them in SetUp is because if I try to then when the class is instantiated it tries to give them a default value using the default constructor, but there is no default constructor for `Expected<TempFile>` so it just gives me an error.



```
For each test defined with TEST_F(), googletest will create a fresh test fixture at runtime, immediately initialize it via SetUp(), run the test, clean up by calling TearDown(), and then delete the test fixture. Note that different tests in the same test suite have different test fixture objects, and googletest always deletes a test fixture before it creates the next one. googletest does not reuse the same test fixture for multiple tests. Any changes one test makes to the fixture do not affect other tests.
```


================
Comment at: libc/test/utils/tools/WrapperGen/wrappergen_test.cpp:48
+    APIArg.append(TargetOS);
+    APIArg.append("/api.td");
+    ProgPath = llvm::StringRef(ToolPath);
----------------
sivachandra wrote:
> If you use the test API file approach, you will not need to construct the path to the API file. Just use the path as listed on the command line.
I'm not sure what you mean by "use the path as listed on the command line", do you mean create a new command line argument for the api to use? Right now I'm still constructing the path, just in a more simple way.


================
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.");
+    }
----------------
abrachet wrote:
> Maybe
> ```lang=CPP
> llvm::ExitOnError errCheck("Temporary file failed to initialize for libc-wrappergen tests.");
> errCheck(STDOutFile);
> errCheck(STDErrFile);
> ```
I tried the code you provided but it gave me the following error: 
`not viable: no known conversion from 'llvm::Expected<llvm::sys::fs::TempFile>' to 'Expected<llvm::sys::fs::TempFile> &&' for 1st argument`
I'll look into it more later but that's why the code is commented out for the moment.


================
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);
----------------
abrachet wrote:
> `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.
yeah that makes this code a lot cleaner. Thank you


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92137