[PATCH] D97403: updated character tests to gtest
Asher Mancinelli via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Mar 1 13:12:35 PST 2021
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
More information about the llvm-commits
mailing list