[libc-commits] [PATCH] D150678: [libc][test] Improve memory check reporting

[Guillaume Chatelet via Phabricator via libc-commits]

gchatelet planned changes to this revision.
gchatelet marked an inline comment as done.
gchatelet added inline comments.


================
Comment at: libc/test/src/string/memcmp_test.cpp:54
     auto span2 = Buffer2.span().subspan(0, size);
-    const bool OK = CheckMemcmp<Impl>(span1, span2, size);
-    if (!OK)
-      testing::tlog << "Failed at size=" << size << '\n';
-    ASSERT_TRUE(OK);
+    ASSERT_TRUE(CheckMemcmp<Impl>(span1, span2, size));
   }
----------------
courbet wrote:
> `ASSERT_TRUE(CheckMemcmp<Impl>(span1, span2, size)) << "size=" << size;` ?
The libc version of `ASSERT_*` and `EXPECT_*` do not allow logging yet.


================
Comment at: libc/test/src/string/memory_utils/memory_check_utils.h:111
 bool CheckBcmp(cpp::span<char> span1, cpp::span<char> span2, size_t size) {
+  const auto error = [=](const char *msg, int cmp,
+                         size_t mismatch_index) -> bool {
----------------
sivachandra wrote:
> gchatelet wrote:
> > sivachandra wrote:
> > > I did not look enough to see if the logging parts are runtime code or test only code. If it is test only, may be add a comment saying that? If not, then a better approach is to use `LIBC_ASSERT`:  https://github.com/llvm/llvm-project/blob/main/libc/src/__support/libc_assert.h
> > > I did not look enough to see if the logging parts are runtime code or test only code. If it is test only, may be add a comment saying that? If not, then a better approach is to use `LIBC_ASSERT`:  https://github.com/llvm/llvm-project/blob/main/libc/src/__support/libc_assert.h
> > 
> > `testing::tlog` is the logging infrastructure that is used in `LibUnitTest` and this file is used in unit tests only (`libc/*test*/src/string/memory_utils/memory_check_utils.h`) so I think this is OK.  Are you suggesting that I comment all `testing::log` lines in this file?
> Sorry, I did not compose my comment correctly. I should have said:
> 
> ```
> ... if the logging parts are **for testing runtime code**...
> ```
> 
> What I meant was, if these messages are for catching test failures, then they should be in some `EXPECT_*` as the raw log messages do not come with line numbers. Or, if they are only providing assert like protection, using a real assert like `LIBC_ASSERT` is better because they also come with line numbers. About comments, if you do not want to use `EXPECT_*` macros, add comments why.
Ha ok : ) Now I understand.

So this is indeed to catch test failures. I did not use `EXPECT_*` for several reasons:
 - Getting the faulty line in this function is not interesting for debugging as you need more context. For instance, the function can be [called several times in one test](https://github.com/llvm/llvm-project/blob/11081a6a1dd7195fb8285e361873e706d6fb6e69/libc/test/src/string/memory_utils/op_tests.cpp#L304-L341) and you really need to know which of the call site is failing (hence the `ASSERT_TRUE` in the test function).
 - The function is usually called in a loop to test many different configurations, `EXPECT_*` will produce a huge log making it hard to identify the root cause. It would need to be an `ASSERT_*` but it is only supposed to be called from the test itself [since it returns](https://github.com/llvm/llvm-project/blob/6012cadc400f4400c97e00da268de17e94a3f5dc/libc/test/UnitTest/LibcTest.h#L411-L413).

Now another solution could be to use `EXPECT_*` here, to return false and to `ASSERT_TRUE` at the call site. But we would still need to log additional information. Currently, `EXPECT_*` and `ASSERT_*` do not allow logging.

I think the solution here is to create custom matchers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150678