[libcxx-commits] [PATCH] D100595: [libcxx][test] Attempt to make debug mode tests more bulletproof

[Kristina Bessonova via Phabricator via libcxx-commits]

krisb added a comment.

In D100595#2730466 <https://reviews.llvm.org/D100595#2730466>, @ldionne wrote:

>> It seems we have another approach for debug mode tests that was added in D59166 <https://reviews.llvm.org/D59166>, which I didn't notice before but which looks much better than we have now (except the facts that it is more complicated, has more requirements, and may suffer from the same problem because most tests don't check for the exact assertion just the fact that an assertion happened). So, the question is why the approach from D59166 <https://reviews.llvm.org/D59166> didn't replace other (I mean the tests which this patch about) debug mode tests? Was the reason in the requirements? Should it finally become the only approach for debug mode tests or it is supposed to coexist with others?
>
> I think it's just that the work was not finished. I'm fine with this patch as-is, I think it's an improvement. If you want to convert everything to D59166 <https://reviews.llvm.org/D59166> style, feel free to do it and you'll have my support too.

I'm still looking at D59166 <https://reviews.llvm.org/D59166> but despite it's a great way to avoid implementing and maintaining dozens (maybe finally we need hundreds) debug tests, it doesn't look like a convenient way to test debug mode on different platforms, especially exotic ones. I'm thinking about something like a test generator that would create individual test files by a given template; it allows keeping the tests small and easy to understand. But I need more time to investigate options.



================
Comment at: libcxx/test/libcxx/containers/sequences/list/list.cons/db_move.pass.cpp:13-15
+// FIXME: this test is supposed to be a regular mode test, not a debug mode one
+// because it contains well-formed code and checks defined behavior
+// (i.e. moving doesn't invalidate iterators).
----------------
Quuxplusone wrote:
> @krisb: D100866 has landed. Either you or I should make a separate PR for adding these tests as regular-mode tests. (I'm happy to do it. I should probably just do it.) I'd like to do that //before// landing this one, so that this one can be smaller.
Great! Do you want me to take care of these then or prefer to do it by yourself?


================
Comment at: libcxx/test/libcxx/containers/unord/unord.map/db_local_iterators_9.pass.cpp:34
     assert(i == c.end(b));
-    ++i;
-    assert(false);
+    EXPECTED_FAIL(++i, "Attempted to increment non-incrementable unordered container local_iterator");
 
----------------
Quuxplusone wrote:
> I notice several of these messages are inconsistently missing the word "a" — "...increment //a// non-incrementable...". Should we fix them? I say yes.
> 
> Orthogonally, I notice there are no matching tests for `--i` "Attempted to //decrement//..." Do we care?
Good points, thank you!

> I notice several of these messages are inconsistently missing the word "a" — "...increment a non-incrementable...". Should we fix them? I say yes.
Yes, and it is definitely better to do before this patch. Are you going to take care of this or want me to fix them?

> Orthogonally, I notice there are no matching tests for --i "Attempted to decrement..." Do we care?
I think yes, it would be good to have such tests. Once this patch is landed I'll prepare a follow-up.


================
Comment at: libcxx/test/support/debug_macros.h:25-31
+#define EXPECTED_FAIL(expr, m)                                                                                         \
+  do {                                                                                                                 \
+    ::expected_libcpp_assert_message = m;                                                                              \
+    std::__libcpp_set_debug_function(&::test_debug_function);                                                          \
+    (void)(expr);                                                                                                      \
+    assert(false);                                                                                                     \
+  } while (false)
----------------
Quuxplusone wrote:
> I think the only remaining controversial point is what to name this macro. I don't really like `EXPECTED_FAIL` (in that it doesn't start with either `ASSERT_` or `TEST_`, and it still reminds me of `XFAIL`, and //technically// it's a reserved identifier because <errno.h>).
> I suggest `ASSERT_FAILS_WITH_MESSAGE(expr, m)` — what do you think of that name, @krisb?
`ASSERT_FAILS_WITH_MESSAGE(expr, m)` is okay to me, but I'd still prefer to start it with `EXPECT`/`EXPECTED`. Maybe we can find something better? Here are some options that came to my mind:

EXPECT_ASSERT_FAIL
EXPECT_LIBCPP_ASSERT_FAIL
EXPECT_ASSERT_FAIL_WITH_MESSAGE

EXPECT_DEATH_WITH_MESSAGE
EXPECT_EXIT_WITH_MESSAGE

EXPECTED_ERROR

LIBCPP_ASSERT_FAIL
LIBCPP_ASSERT_FAIL_WITH_MESSAGE

If nothing looks good to you, I'll stay with your variant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100595