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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a comment.

LGTM % comments, but I'd like to
(A) wait until either-Kris-or-I have split off the tests-that-really-aren't-debug-mode-tests into a separate PR, and also
(B) see it updated to search-and-replace `EXPECTED_FAIL` into `ASSERT_FAILS_WITH_MESSAGE` (or something nicer, if we can find something nicer) and pass buildkite again



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


================
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");
 
----------------
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?


================
Comment at: libcxx/test/libcxx/containers/unord/unord.set/db_insert_hint_const_lvalue.pass.cpp:29-30
     P v(3.5);
-    R r = c.insert(e, v);
-    assert(false);
+    EXPECTED_FAIL(c.insert(e, v), "unordered_set::insert(const_iterator, const value_type&) called with an iterator "
+                                  "not referring to this unordered_set");
 
----------------
Nit: I would prefer not to break the string literal, so that it's easier to grep for. Break the line as
```
    EXPECTED_FAIL(c.insert(e, v),
        "unordered_set::insert(const_iterator, const value_type&) called with an iterator not referring to this unordered_set");
```
and ignore clang-format's griping.


================
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)
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100595