Page MenuHomePhabricator

[libc] add tests to WrapperGen
ClosedPublic

Authored by michaelrj on Nov 25 2020, 2:41 PM.

Details

Summary

This adds an initial test that can serve as a basis for other tests on
wrappergen.

Diff Detail

Event Timeline

michaelrj created this revision.Nov 25 2020, 2:41 PM
michaelrj requested review of this revision.Nov 25 2020, 2:41 PM

I think a lit test might be more appropriate. I had started setting this up a while ago in D75427, it might not be worth setting up for what will be probably just one test though.

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

sivachandra added inline comments.Nov 30 2020, 12:50 PM
libc/test/utils/CMakeLists.txt
3

Should there be an additional nesting tools/WrapperGen?

libc/test/utils/WrapperGen/CMakeLists.txt
1 ↗(On Diff #307716)

The implementation is currently wrappergen specific. So, may be make it generic and move it up one level higher under libc/test/utils/tools?

10 ↗(On Diff #307716)

Add ARGS or something equivalent - See below.

29 ↗(On Diff #307716)

Instead of listing --path, get the command line args from the arguments to add_libc_tool_unittest.

30 ↗(On Diff #307716)

Give a generic name like, say --tool?

31 ↗(On Diff #307716)

Same as for --path.

45 ↗(On Diff #307716)

This should be listed under DEPENDS of add_libc_tool_unittest?

48 ↗(On Diff #307716)

You can move lines 47 and 48 into add_libc_tool_unittest function.

Also, use fully qualified names: https://github.com/llvm/llvm-project/blob/master/libc/cmake/modules/LLVMLibCTargetNameUtils.cmake

libc/test/utils/WrapperGen/wrappergen_test.cpp
27 ↗(On Diff #307716)

Instead of this, we should perhaps have an option called apifile for which we pass in a test API file. This test file need not do anything fancy. Something like:

include "spec/spec.td"
include "spec/std.td"
70 ↗(On Diff #307716)

Also add a test for aliases.

105 ↗(On Diff #307716)

Can you list the brittle points and add TODOs separately for them?

michaelrj updated this revision to Diff 308491.Nov 30 2020, 3:25 PM
michaelrj marked 7 inline comments as done.

address most of the comments

submit comments

libc/test/utils/WrapperGen/CMakeLists.txt
45 ↗(On Diff #307716)

no, if libc-wrappergen is part of the DEPENDS block it tries to link it in as part of the tests, which causes a bunch of errors, thus it's added separately.

48 ↗(On Diff #307716)

I'm not quite sure what you mean by "use fully qualified names" since the file you linked appears to only support paths inside libc and LLVMSupport is from https://github.com/llvm/llvm-project/blob/master/llvm/lib/Support/CMakeLists.txt#L79 unless you meant a different name?

sivachandra added inline comments.Nov 30 2020, 4:05 PM
libc/test/utils/WrapperGen/CMakeLists.txt
45 ↗(On Diff #307716)

So, IIUC, add_libc_tool_unittest expects an optional DEPENDS argument but does nothing with it. If DEPENDS is confusing, use TOOL_TARGET may be and then you can call add_dependencies to add this tool as a dependency.

48 ↗(On Diff #307716)

What I mean is, the wrappergen_test target should have a fully qualified name like libc.test.utils.tools.WrapperGen.wrappergen_test.

libc/test/utils/tools/CMakeLists.txt
3
37

Fix.

libc/test/utils/tools/WrapperGen/CMakeLists.txt
7

Why list an empty DEPENDS?

libc/test/utils/tools/WrapperGen/wrappergen_test.cpp
28

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?

49

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 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

libc/test/utils/tools/WrapperGen/wrappergen_test.cpp
37–40

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

52–57

Maybe

llvm::ExitOnError errCheck("Temporary file failed to initialize for libc-wrappergen tests.");
errCheck(STDOutFile);
errCheck(STDErrFile);
84–91

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.

122

Not needed. Nor are the brackets on these 1 line while statements

michaelrj updated this revision to Diff 308818.Dec 1 2020, 4:55 PM
michaelrj marked 7 inline comments as done.

add testapi.td so that the tests are no longer platform dependent and clean up the code a little.

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.

libc/test/utils/tools/WrapperGen/CMakeLists.txt
7

it was a leftover from when I was trying adding libc-wrappergen as a full dependency.

libc/test/utils/tools/WrapperGen/wrappergen_test.cpp
28

no problems, I just hadn't done it yet. It's now done and at test/utils/tools/WrapperGen/testapi.td

37–40

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.
49

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.

52–57

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.

84–91

yeah that makes this code a lot cleaner. Thank you

michaelrj updated this revision to Diff 308819.Dec 1 2020, 4:58 PM

remove references to targetos

Harbormaster completed remote builds in B80740: Diff 308818.
sivachandra added inline comments.Dec 2 2020, 8:28 AM
libc/test/utils/tools/WrapperGen/wrappergen_test.cpp
37–40

In a later pass, we should move constructs like this into a tool-test-matcher which will make this look a lot more cleaner.

49

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.

michaelrj updated this revision to Diff 309077.Dec 2 2020, 3:22 PM
michaelrj marked 2 inline comments as done.

added more tests

submit comments

libc/test/utils/tools/WrapperGen/wrappergen_test.cpp
49

I do still need the path to the libc directory since that's an argument for wrappergen, but I have added a new argument for the api file.

sivachandra accepted this revision.Dec 3 2020, 3:15 PM

I think we can chisel this out to be a little more cleaner but we can do it in a later pass.

libc/test/utils/tools/WrapperGen/testapi.td
3

Fix.

libc/test/utils/tools/WrapperGen/wrappergen_test.cpp
248

Fix.

This revision is now accepted and ready to land.Dec 3 2020, 3:15 PM
michaelrj updated this revision to Diff 309570.Dec 4 2020, 10:08 AM
michaelrj marked an inline comment as done.

final cleanup

This revision was landed with ongoing or failed builds.Dec 4 2020, 10:14 AM
This revision was automatically updated to reflect the committed changes.
michaelrj marked an inline comment as done.