[PATCH] D97403: updated character tests to gtest

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added a comment.

Thank you for addressing my comments @ashermancinelli ! This looks really good and provides more test coverage than `flang/unittests/Runtime/character.cpp`, that's great! Before this is merged, could you delete the original test file?

I've left a few more suggestions, but nothing major and mostly nits.



================
Comment at: flang/unittests/RuntimeGTest/CharacterTest.cpp:11-13
+/// \brief Basic sanity tests of CHAR API; exhaustive testing will be done
+/// in Fortran. Similar testing infrastructure also exists in the parallel
+/// directory `flang/unittests/Runtime` while we port those tests to use GTest.
----------------
A couple of nits:

[nit1] `exhaustive testing will be done in Fortran.` - I would leave this out (it's a comment about things happening elsewhere in the codebase - it's unlikely that we will remember to update this comment once that `exhaustive` testing happens)

[nit2] `Similar testing infrastructure also exists in the parallel directory` - this won't be needed if tests are not duplicated, right?


================
Comment at: flang/unittests/RuntimeGTest/CharacterTest.cpp:26
+
+TEST(CharacterTests, AppendAndPad) {
+  static constexpr int limitMax{8};
----------------
[nit] I would be tempted to separate this and tests for `CharacterCompareScalar{n}` with a big comment, e.g.:
```
//-----------------------------------------------------------------------
/// Tests (and infra) for CharacterAppend1 and CharacterPad1
//------------------------------------------------------------------------
```


================
Comment at: flang/unittests/RuntimeGTest/CharacterTest.cpp:34
+    xLen = RTNAME(CharacterAppend1)(x, limit, xLen, "DE", 2);
+    ASSERT_LE(xLen, limit) << "xLen " << xLen << ">" << limit;
+    RTNAME(CharacterPad1)(x, limit, xLen);
----------------
1. There's quite a few checks in this test - would it be helpful to add a short comment highlighting _why_ or _what_ is validated?

2. Would it make sense to add a corner-case test? For example:
```
// Verify that the limit is not exceeded
xLen = RTNAME(CharacterAppend1)(x, limit, xLen, "123456789", 9);
ASSERT_EQ(xLen, limit) << "xLen " << xLen << ">" << limit;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97403