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

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

Thanks for the fast turnaround!



================
Comment at: llvm/utils/gn/secondary/llvm/utils/unittest/unittest.gni:34
+    # out/gn/obj/clang/unittests/Format/FormatTests, which seems fine.
+    output_dir = target_out_dir
+    deps += [
----------------
phosek wrote:
> 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 `unittest`s is unsupported.
Good point, done.


================
Comment at: llvm/utils/gn/secondary/llvm/utils/unittest/unittest.gni:35
+    output_dir = target_out_dir
+    deps += [
+      "//llvm/utils/unittest/UnitTestMain",
----------------
phosek wrote:
> 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.
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 :-)


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

https://reviews.llvm.org/D56116