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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
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");
----------------
michaelrj wrote:
> 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.
> ```
In a later pass, we should move constructs like this into a //tool-test-matcher// which will make this look a lot more cleaner.


================
Comment at: libc/test/utils/tools/WrapperGen/wrappergen_test.cpp:48
+    APIArg.append(TargetOS);
+    APIArg.append("/api.td");
+    ProgPath = llvm::StringRef(ToolPath);
----------------
michaelrj wrote:
> 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.
Yes, instead of the command line argument which is expecting a path to the libc directory, I am suggesting to use an argument which takes a path to the API file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92137