[libcxx-commits] [PATCH] D142808: [libc++][test] Adds more generic test macros.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 11 inline comments as done.
Mordante added a comment.

Thanks for the review. I will look at the other comments after testing locally.



================
Comment at: libcxx/docs/TestingLibcxx.rst:170-172
+- ``utils/libcxx/test`` This directory contains the testing infrastructure that
+  configures LIT testing. For example, when configuring libc++ without
+  locales in CMake it sets the appropriate LIT test feature.
----------------
ldionne wrote:
> I would avoid documenting that, since it's only useful for frequent contributors and they pretty much already know where to find those. I am a bit worried about some of this information becoming out of date.
I'm a bit on the fence. It's indeed most useful for frequent contributors, but not documented makes it harder for new contributors to become frequent contributors. Since I already have a link to `libcxx/utils/libcxx/test/format.py` it's quite easy to find this directory. So I'll remove it.


================
Comment at: libcxx/docs/TestingLibcxx.rst:303-309
+TEST_IS_UNSUPPORTED
+^^^^^^^^^^^^^^^^^^^
+
+An alias for ``return``. This can be used to document a part of the tests is
+not supported on some platforms or with some configurations. For example
+several filesystem permissions differ between Windows and POSIX platforms,
+therefore some of parts of the permission tests are not done on Windows.
----------------
ldionne wrote:
> This looks like a footgun to me, I would not provide that. Hiding this sort of mechanism behind a macro seems like a bad idea to me.
> 
> I would instead `return` manually and add a comment. This isn't great, but the real solution would be to have a way to integrate this into `lit` so that `lit` can show us the test as being `UNSUPPORTED` based on a runtime condition, which is a much more involved thing to do.
As mentioned in our 1:1, this is added as compatibility with the existing rapid tests. As agreed I will remove this and update the test using this macro to use that approach.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142808