[PATCH] D97403: updated character tests to gtest

[Asher Mancinelli via Phabricator via llvm-commits]

ashermancinelli added inline comments.


================
Comment at: flang/unittests/RuntimeGTest/CharacterTest.cpp:26
+
+TEST(CharacterTests, AppendAndPad) {
+  static constexpr int limitMax{8};
----------------
awarzynski wrote:
> [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
> //------------------------------------------------------------------------
> ```
If we're already breaking the tests up into smaller units, would we be better served to break tests into two files?


================
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);
----------------
awarzynski wrote:
> 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;
> ```
Both counts sound good to me!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97403