This is an archive of the discontinued LLVM Phabricator instance.

[gn build] Make `ninja check-clang` also run Clang's unit tests
ClosedPublic

Authored by thakis on Dec 27 2018, 5:06 PM.

Details

Summary

Also add a build file for clang/lib/ASTMatchers/Dynamic, which is only needed by tests (and clang/tools/extra).

Also make llvm/utils/gn/build/sync_source_lists_from_cmake.py check that every CMakeLists.txt file below {lld,clang}/unittests has a corresponding BUILD.gn file, so we notice if new test binaries get added (since the failure mode for missing GN build files for tests is just the tests silently not running in the GN build).

Also add a unittest() macro for defining unit test targets, and add a lengthy comment there about where the unit test binaries go and why.

With this, the build files for //clang are complete.

Diff Detail

Repository
rL LLVM

Event Timeline

thakis created this revision.Dec 27 2018, 5:06 PM
thakis edited the summary of this revision. (Show Details)Dec 27 2018, 5:07 PM
thakis updated this revision to Diff 179667.Dec 28 2018, 4:12 PM
thakis edited the summary of this revision. (Show Details)
  • add test BUILD.gn support to sync script
  • remove bigobj fixme since that issue isn't specific to that file
thakis updated this revision to Diff 179736.Dec 30 2018, 10:31 AM
thakis edited the summary of this revision. (Show Details)

Add unittest template. This is now ready to go.

thakis edited the summary of this revision. (Show Details)Dec 30 2018, 10:32 AM
phosek added inline comments.Dec 30 2018, 2:36 PM
llvm/utils/gn/secondary/llvm/utils/unittest/unittest.gni
34 ↗(On Diff #179736)

What if someone explicitly sets output_dir in the invoker? We should either preserve that value or assert with an error message that overriding output_dir in unittests is unsupported.

35 ↗(On Diff #179736)

You should check if deps is defined and set deps = [] if not, otherwise the attempt to add to non-existent list is going to fail.

thakis updated this revision to Diff 179744.Dec 30 2018, 3:36 PM
thakis marked 2 inline comments as done.

comments

Thanks for the fast turnaround!

llvm/utils/gn/secondary/llvm/utils/unittest/unittest.gni
34 ↗(On Diff #179736)

Good point, done.

35 ↗(On Diff #179736)

I can't imagine that anyone would want a unit test that doesn't at least depend on the code under test. I agree in principle, but I think in practice this would be dead code 100% of the time. Might as well omit it then :-)

phosek accepted this revision.Dec 30 2018, 3:41 PM

LGTM

This revision is now accepted and ready to land.Dec 30 2018, 3:41 PM
This revision was automatically updated to reflect the committed changes.