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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/docs/TestingLibcxx.rst:153
+
+- ``test/support`` This directory contains several helper headers with generic
+  parts for the tests. The most important header is ``test_macros.h``. This file
----------------



================
Comment at: libcxx/docs/TestingLibcxx.rst:156-158
+  similar to the ``__config`` file in libc++'s ``include`` directory. Since
+  libc++'s tests are used by other Standard libraries the ``TEST_FOO`` macros
+  should be using instead of the ``_LIBCPP_FOO`` macros.
----------------



================
Comment at: libcxx/docs/TestingLibcxx.rst:159
+  should be using instead of the ``_LIBCPP_FOO`` macros.
+- ``test/std`` This directory contains the tests that validate the library under
+  test conforms to the C++ Standard. The paths and the names of the test match
----------------
Etc.. It's less confusing for people if we consistently use paths from the root of the monorepo.


================
Comment at: libcxx/docs/TestingLibcxx.rst:162-163
+  the section names in the C++ Standard. Note that the C++ Standard sometimes
+  reorganises its structure, therefore some tests are at their historical
+  location instead of their current location.
+- ``test/libcxx`` This directory contains the tests that validate libc++
----------------



================
Comment at: libcxx/docs/TestingLibcxx.rst:164-169
+- ``test/libcxx`` This directory contains the tests that validate libc++
+  specific implementation details. For example libc++ has "wrapped
+  iterators", that allow doing bounds checks. These are tested in this
+  directory Tests that mainly test Standard conformance and a few libc++
+  specific parts are in the ``test/std`` directory.  The structure of this
+  directories follows the structure of ``test/std``.
----------------



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


================
Comment at: libcxx/docs/TestingLibcxx.rst:228
+   the ``stderr``,
+ - else it is invoked without any additional arguments.
+
----------------



================
Comment at: libcxx/docs/TestingLibcxx.rst:231
+This makes it possible to write additional information when a test fails,
+either by supplying a hard-coded string or generate it at runtime. For example
+
----------------



================
Comment at: libcxx/docs/TestingLibcxx.rst:279
+  void test_excption([[maybe_unused]] int arg) {
+  #ifndef TEST_HAS_NO_EXCEPTIONS // Do nothing when tests are disables
+    try {
----------------



================
Comment at: libcxx/docs/TestingLibcxx.rst:282
+      foo(arg);
+      assert(false); // validates foo really throws.
+    } catch ([[maybe_unused]] const bar& e) {
----------------



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


================
Comment at: libcxx/test/support/assert_macros.h:38
 
-#if TEST_STD_VER > 17
-
-#  ifndef TEST_HAS_NO_LOCALIZATION
-template <class T>
-concept test_char_streamable = requires(T&& value) { std::stringstream{} << std::forward<T>(value); };
-#  endif
-
-// If possible concatenates message for the assertion function, else returns a
-// default message. Not being able to stream is not considered and error. For
-// example, streaming to std::wcerr doesn't work properly in the CI. Therefore
-// the formatting tests should only stream to std::string_string.
-template <class... Args>
-std::string test_concat_message([[maybe_unused]] Args&&... args) {
-#  ifndef TEST_HAS_NO_LOCALIZATION
-  if constexpr ((test_char_streamable<Args> && ...)) {
-    std::stringstream sstr;
-    ((sstr << std::forward<Args>(args)), ...);
-    return sstr.str();
-  } else
-#  endif
-    return "Message discarded since it can't be streamed to std::cerr.\n";
+void test_log(const char* condition, const char* file, int line, std::string&& message) {
+  test_log(condition, file, line, message.c_str());
----------------
I don't think you actually need the `string` overload. That would allow you to drop the header dependency.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142808