[flang] update character tests to use gtest

[ashermancinelli]

Summary

Move character tests to gtest, according to reviews from revision
D97349. Create a new temporary directory parallel to old runtime
unittests directory to facilitate the transition. Once patches for all
tests have been accepted using GTest, the old directory may be removed.
The new directory is required because LLVM's CMake unit test
infrastructure requires that the test depends on all source files in
the CMAKE_CURRENT_SOURCE_DIR directory.

[ashermancinelli]

I no longer modify anything in the old runtime tests directory, so this patch should be much more straightforward.

[awarzynski]

Hi @ashermancinelli ,

Thank you for working on this! I quickly scanned through the test coverage is identical to flang/unittests/Runtime/character.cpp. What follows are some ideas for fine-tuning.

• Please don't hesitate to improve test coverage if you see some low hanging fruits :)
• Please remember about file headers
• The logic in RuntimeTesting.cpp is a bit complex - is there any chance that that's tested too? (in particular, CatchCrash, perhaps by triggering a call to Crash)
• I am also thinking that since procedures from character.cpp don't crash (at least the ones that you test), we don't really need CatchCrash just yet (though it would be great to see it being tested). Perhaps the test fixture could be simplified?
• IMO we should avoid duplicating code/tests. I recommend deleting similar tests from flang/unittests/Runtime.

More comments inline.

flang/unittests/RuntimeGTest/CharacterTest.cpp

18

• this can be declared outside the loop
• both here and in the loop, 8 is a magic number that's re-used and that could be replaced with a variable

24

IIUC, CharacterPad1 does not affect xlen, so this check could be moved before L22.

25–27

Perhaps:

EXPECT_NE(x[limit], `\0`)  << "x[" << limit << "]='" << x[limit] << "'\n";

IMHO this would be clearer.

29

IIUC, at this point x = abcDE and this memcpy simply overwrites x with similar value?

I think that std::memcpy either succeeds or we get UB. So this ASSERT_FALSE does not really add much, does it?

48

[nice-to-have] CharacterCompareScalar2, CharacterCompareScalar3, CharacterCompareScalar4 :)

53

Why not use x and y instead of buf[0] and buf[1], respectively? Doesn't ASSERT_EQ() << behave like operator<< for std::ostream. So there should be no need to convert x and y to character arrays? Am I missing something?

64–68

[nit] Personally I like to put ASSERT_ and EXPECT_ in the main test method (like this one). IMO it makes it much clearer what is actually being tested. But it's a matter of preference!

flang/unittests/RuntimeGTest/RuntimeTesting.h

17 ↗

(On Diff #326149)

This is currently only used in RuntimeTesting.cpp. Perhaps move there?

[ashermancinelli]

Thank you for the review! It appears that many of your comments are due to lack of attention to detail when moving the test bodies over from the old runtime tests directory to this one (eg your comments on lines 28 and 52) - I'll revisit all of your comments and see where I can improve upon the old tests as well.

flang/unittests/RuntimeGTest/CharacterTest.cpp

64–68

I agree, but do you have a suggestion for how to move the assertions/expectations into the body of the test without code repetition due to the parameter swapping? I'm happy to field suggestions on this one :)

flang/unittests/RuntimeGTest/RuntimeTesting.h

17 ↗

(On Diff #326149)

Correct, however this will be used in other tests as they are moved over from the old runtime tests so I think it could either remain here or be removed altogether.

[ashermancinelli]

flang/unittests/RuntimeGTest/CharacterTest.cpp

29

memcmp, not memcpy

[ashermancinelli]

flang/unittests/RuntimeGTest/CharacterTest.cpp

25–27

I agree, but this should be

EXPECT_EQ(x[limit], '\0') << "x[" << limit << "]='" << x[limit] << "'";

correct?

[awarzynski]

flang/unittests/RuntimeGTest/CharacterTest.cpp

25–27

+1

29

:facepalm: Thank you!

64–68

IMO this is a borderline situation. Yes, code re-use is nice. But so is making code & tests easier to follow. Which should we prioritize?

Also, I left a comment for DoCharacterComparison - I think that that method can be simplified, so there won't be that much code to re-use.

I marked this as a [nit] as IMO it's quite subjective. I'm happy with whatever you decide or propse!

flang/unittests/RuntimeGTest/RuntimeTesting.h

17 ↗

(On Diff #326149)

Then I suggest removing (so that the patch only contains the required stuff)

[ashermancinelli]

flang/unittests/RuntimeGTest/CharacterTest.cpp

48

I don't think this would be too hard with some template shenanigans... I'll add it in the next patch.

[ashermancinelli]

In D97403#2588156, @awarzynski wrote:

I am also thinking that since procedures from character.cpp don't crash (at least the ones that you test), we don't really need CatchCrash just yet (though it would be great to see it being tested). Perhaps the test fixture could be simplified?

Sure - it seems the existing method of crash recovery (exceptions) is the only way to go here since crash std::aborts, so we'll have to redo this part anyways.

[awarzynski]

In D97403#2589137, @ashermancinelli wrote:

Sure - it seems the existing method of crash recovery (exceptions) is the only way to go here since crash std::aborts, so we'll have to redo this part anyways.

Wouldn't death tests work here?

[ashermancinelli]

• Templated character tests over all char types
• Removed unneeded fixture
• Documentation and better naming

[ashermancinelli]

flang/unittests/RuntimeGTest/CharacterTest.cpp

97–98

An alternative solution to the CreateParameters function would be a static tuple of type tuple<vector<PARAMETER<char>>, vector<PARAMETER<char16_t>>, ...>. This way, initializing parameters would just be std::get<CHAR>(AnswerKey) or something similar. If anyone thinks this or another solution would be better than what I've used here, please let me know.

[awarzynski]

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.

flang/unittests/RuntimeGTest/CharacterTest.cpp

12–14

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?

27

[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
//------------------------------------------------------------------------

35

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;

[ashermancinelli]

flang/unittests/RuntimeGTest/CharacterTest.cpp

27

If we're already breaking the tests up into smaller units, would we be better served to break tests into two files?

35

Both counts sound good to me!

[ashermancinelli]

Updated documentation, added a new test case based on conversation in the previous diff, and removed old character tests.

[awarzynski]

LGTM, thank you!

flang/unittests/RuntimeGTest/CharacterTest.cpp

27

Both files would be quite small - personally I'd keep it here. But I won't object if you prefer to split.

124–126

Btw, I thought that we could get rid of this and now I see that we cannot. My bad, apologies! But I don't think that the code-duplication here is bad. IMO it actually makes this easier to read. But I appreciate that that's subjective :)

[awarzynski]

Hi @ashermancinelli ,

I've just noticed that character.cpp has been updated in https://reviews.llvm.org/D97580 in the meantime. Would you be able to re-base?

Also, CC @klausler who touched that file and @jeanPerier who reviewed D97580. Apologies for missing you earlier!

[ashermancinelli]

@awarzynski Sure! I'll add corresponding tests and have another patch up soon.

[ashermancinelli]

I've added tests for scan and verify functions, and cleaned up the comparison tests. I think the std::get method is a bit cleaner than what we had before, but I would love to hear other opinions on this.

[ashermancinelli]

flang/unittests/RuntimeGTest/CharacterTest.cpp

198

AFAIK, char_traits<>::length is exactly the same as strlen but I can't verify this on cppref. If someone can verify that would be great.

264–272

The tests for VerifyX and ScanX are identical and could potentially be merged into one test case. I think this may increase the work needed to add test cases in the future so I kept them separate, but please feel free to suggest alternatives.

[awarzynski]

Thank you for updating this, @ashermancinelli! You've increased the test coverage for Verify{N} and Scan{N} methods - that's great to see.

I agree with your design decisions and I don't see any obvious ways to improve this. IMHO, this implementation is relatively easy to follow - you've done a really good job here!

I've just realized one thing. ALL_CAPS is usually reserved for macros (though I couldn't find this in LLVM's coding style docs). When specifying type aliases with using, I believe that we should use CamelCase instead. That's based on what's seem to be most widely used inside Flang. So, for example,

• VerifyParameterTy instead of VERIFY_PARAMETER
• VerifyFuncTy instead of VERIFY_FUNC

Apologies for not raising this earlier. I really liked your original approach, but we should follow the existing precedent in the project (I couldn't find any explicit notes on using, neither here nor here).

Few more comments/suggestions inline.

flang/unittests/RuntimeGTest/CharacterTest.cpp

58

Perhaps ComparisonFuncTy?

62

[nit] Would it be worth adding a comment here to improve visibility e.g. The list of CharacterCompareScalar{N} functions tested here? It's the key bit, isn't it? Same in other places.

71

[nit] PARAMETER --> PARAMETERS (same in other places)

154

ScanParameterTy instead?

198

Yes, that's how I read this.

[ashermancinelli]

@awarzynski

ALL_CAPS is usually reserved for macros...
but we should follow the existing precedent in the project...

I'm used to camel case for template args as well and I too searched the documentation for guidance, but not finding a clear policy I tried to follow the conventions in the file being tested:

flang/runtime/character.cpp:

template <typename CHAR, bool ADJUSTR>
static void Adjust(CHAR *to, const CHAR *from, std::size_t chars) {

Do you think we should break from conventions in the testee? Personally, I too prefer camelcase, but I'll defer to you on this one 🙂

[tskeith]

In general, we've been using ALL_CAPS for template parameters and CamelCase for type names.

flang/unittests/RuntimeGTest/CharacterTest.cpp

222

I suggest: using VerifyParameter = ....

227

This should be verifyParameters.

It might be more readable to give this type a name if you can come up with a good one.

[ashermancinelli]

I've updated the revision - thanks for your comments @awarzynski and @tskeith! Please let me know what you think of the naming conventions I went with in this revision.

flang/unittests/RuntimeGTest/CharacterTest.cpp

154

I like that 👍

222

Thanks for the suggestions!

[ashermancinelli]

Updated naming conventions and documentation thanks to comments and suggestions from @awarzynski and @tskeith.

[awarzynski]

Thank you for clarifying, @tskeith.

LGTM @ashermancinelli !

flang/unittests/CMakeLists.txt

if(NOT ARG_SLOW_TEST)
     add_dependencies(FlangUnitTests ${test_name}${suffix})
   endif()
 endfunction()
 
 add_subdirectory(Optimizer)
 add_subdirectory(Decimal)
 add_subdirectory(Evaluate)
 add_subdirectory(Runtime)
+add_subdirectory(RuntimeGTest)
 
 if (FLANG_BUILD_NEW_DRIVER)
   add_subdirectory(Frontend)
 endif()

flang/unittests/Runtime/CMakeLists.txt

add_flang_nongtest_unittest(external-io
   RuntimeTesting
   FortranRuntime
 )
 
 add_flang_nongtest_unittest(list-input
   RuntimeTesting
   FortranRuntime
 )
-
-add_flang_nongtest_unittest(character
-  RuntimeTesting
-  FortranRuntime
-)

flang/unittests/Runtime/character.cpp

-// Basic sanity tests of CHARACTER API; exhaustive testing will be done
-// in Fortran.
-
-#include "../../runtime/character.h"
-#include "testing.h"
-#include <cstring>
-
-using namespace Fortran::runtime;
-
-static void AppendAndPad(std::size_t limit) {
-  char x[8];
-  std::size_t xLen{0};
-  std::memset(x, 0, sizeof x);
-  xLen = RTNAME(CharacterAppend1)(x, limit, xLen, "abc", 3);
-  xLen = RTNAME(CharacterAppend1)(x, limit, xLen, "DE", 2);
-  RTNAME(CharacterPad1)(x, limit, xLen);
-  if (xLen > limit) {
-    Fail() << "xLen " << xLen << ">" << limit << '\n';
-  }
-  if (x[limit]) {
-    Fail() << "x[" << limit << "]='" << x[limit] << "'\n";
-    x[limit] = '\0';
-  }
-  if (std::memcmp(x, "abcDE   ", limit)) {
-    Fail() << "x = '" << x << "'\n";
-  }
-}
-
-static void TestCharCompare(const char *x, const char *y, std::size_t xBytes,
-    std::size_t yBytes, int expect) {
-  int cmp{RTNAME(CharacterCompareScalar1)(x, y, xBytes, yBytes)};
-  if (cmp != expect) {
-    char buf[2][8];
-    std::memset(buf, 0, sizeof buf);
-    std::memcpy(buf[0], x, xBytes);
-    std::memcpy(buf[1], y, yBytes);
-    Fail() << "compare '" << buf[0] << "'(" << xBytes << ") to '" << buf[1]
-           << "'(" << yBytes << "), got " << cmp << ", should be " << expect
-           << '\n';
-  }
-}
-
-static void Compare(const char *x, const char *y, std::size_t xBytes,
-    std::size_t yBytes, int expect) {
-  TestCharCompare(x, y, xBytes, yBytes, expect);
-  TestCharCompare(y, x, yBytes, xBytes, -expect);
-}
-
-int main() {
-  StartTests();
-  for (std::size_t j{0}; j < 8; ++j) {
-    AppendAndPad(j);
-  }
-  Compare("abc", "abc", 3, 3, 0);
-  Compare("abc", "def", 3, 3, -1);
-  Compare("ab ", "abc", 3, 2, 0);
-  Compare("abc", "abc", 2, 3, -1);
-  return EndTests();
-}

flang/unittests/RuntimeGTest/CMakeLists.txt

+add_flang_unittest(FlangRuntimeTests
+  CharacterTest.cpp
+)
+
+target_link_libraries(FlangRuntimeTests
+  PRIVATE
+  FortranRuntime
+)

flang/unittests/RuntimeGTest/CharacterTest.cpp

+//===-- flang/unittests/RuntimeGTest/CharacterTest.cpp ----------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "../../runtime/character.h"
+#include "gtest/gtest.h"
+#include <cstring>
+#include <functional>
+#include <tuple>
+#include <vector>
+
+using namespace Fortran::runtime;
+
+TEST(CharacterTests, AppendAndPad) {
+  static constexpr int limitMax{8};
+  static char buffer[limitMax];
+  static std::size_t offset{0};
+  for (std::size_t limit{0}; limit < limitMax; ++limit, offset = 0) {
+    std::memset(buffer, 0, sizeof buffer);
+
+    // Ensure appending characters does not overrun the limit
+    offset = RTNAME(CharacterAppend1)(buffer, limit, offset, "abc", 3);
+    offset = RTNAME(CharacterAppend1)(buffer, limit, offset, "DE", 2);
+    ASSERT_LE(offset, limit) << "offset " << offset << ">" << limit;
+
+    // Ensure whitespace padding does not overrun limit, the string is still
+    // null-terminated, and string matches the expected value up to the limit.
+    RTNAME(CharacterPad1)(buffer, limit, offset);
+    EXPECT_EQ(buffer[limit], '\0')
+        << "buffer[" << limit << "]='" << buffer[limit] << "'";
+    buffer[limit] = buffer[limit] ? '\0' : buffer[limit];
+    ASSERT_EQ(std::memcmp(buffer, "abcDE   ", limit), 0)
+        << "buffer = '" << buffer << "'";
+  }
+}
+
+TEST(CharacterTests, CharacterAppend1Overrun) {
+  static constexpr int bufferSize{4};
+  static constexpr std::size_t limit{2};
+  static char buffer[bufferSize];
+  static std::size_t offset{0};
+  std::memset(buffer, 0, sizeof buffer);
+  offset = RTNAME(CharacterAppend1)(buffer, limit, offset, "1234", bufferSize);
+  ASSERT_EQ(offset, limit) << "CharacterAppend1 did not halt at limit = "
+                           << limit << ", but at offset = " << offset;
+}
+
+//------------------------------------------------------------------------------
+/// Tests and infrastructure for character comparison functions
+//------------------------------------------------------------------------------
+
+template <typename CHAR>
+using CHARCOMP_FUNC =
+    std::function<int(const CHAR *, const CHAR *, std::size_t, std::size_t)>;
+
+// Maps character types to character comparison functions
+template <typename T> CHARCOMP_FUNC<T> CreatecharacterComparisonFunc();
+
+template <> CHARCOMP_FUNC<char> CreatecharacterComparisonFunc<char>() {
+  return RTNAME(CharacterCompareScalar1);
+}
+
+template <> CHARCOMP_FUNC<char16_t> CreatecharacterComparisonFunc<char16_t>() {
+  return RTNAME(CharacterCompareScalar2);
+}
+
+template <> CHARCOMP_FUNC<char32_t> CreatecharacterComparisonFunc<char32_t>() {
+  return RTNAME(CharacterCompareScalar4);
+}
+
+// Types of _values_ over which tests are parameterized
+template <typename CHAR>
+using PARAMETER = std::tuple<const CHAR *, const CHAR *, int, int, int>;
+
+template <typename T> std::vector<PARAMETER<T>> CreateParameters();
+
+template <> std::vector<PARAMETER<char>> CreateParameters<char>() {
+  return {
+      std::make_tuple("abc", "abc", 3, 3, 0),
+      std::make_tuple("abc", "def", 3, 3, -1),
+      std::make_tuple("ab ", "abc", 3, 2, 0),
+      std::make_tuple("abc", "abc", 2, 3, -1),
+  };
+}
+
+template <> std::vector<PARAMETER<char16_t>> CreateParameters<char16_t>() {
+  return {
+      std::make_tuple(u"abc", u"abc", 3, 3, 0),
+      std::make_tuple(u"abc", u"def", 3, 3, -1),
+      std::make_tuple(u"ab ", u"abc", 3, 2, 0),
+      std::make_tuple(u"abc", u"abc", 2, 3, -1),
+  };
+}
+
+template <> std::vector<PARAMETER<char32_t>> CreateParameters<char32_t>() {
+  return {
+      std::make_tuple(U"abc", U"abc", 3, 3, 0),
+      std::make_tuple(U"abc", U"def", 3, 3, -1),
+      std::make_tuple(U"ab ", U"abc", 3, 2, 0),
+      std::make_tuple(U"abc", U"abc", 2, 3, -1),
+  };
+}
+
+template <typename CHAR>
+struct CharacterComparisonTests : public ::testing::Test {
+  CharacterComparisonTests()
+      : parameters(CreateParameters<CHAR>()),
+        characterComparisonFunc(CreatecharacterComparisonFunc<CHAR>()) {}
+  std::vector<PARAMETER<CHAR>> parameters;
+  CHARCOMP_FUNC<CHAR> characterComparisonFunc;
+};
+
+using CHARACTERS = ::testing::Types<char, char16_t, char32_t>;
+TYPED_TEST_CASE(CharacterComparisonTests, CHARACTERS);
+
+TYPED_TEST(CharacterComparisonTests, CompareCharacters) {
+  for (auto &[x, y, xBytes, yBytes, expect] : this->parameters) {
+    int cmp{this->characterComparisonFunc(x, y, xBytes, yBytes)};
+    TypeParam buf[2][8];
+    std::memset(buf, 0, sizeof buf);
+    std::memcpy(buf[0], x, xBytes);
+    std::memcpy(buf[1], y, yBytes);
+    ASSERT_EQ(cmp, expect) << "compare '" << x << "'(" << xBytes << ") to '"
+                           << y << "'(" << yBytes << "), got " << cmp
+                           << ", should be " << expect << '\n';
+
+    // Perform the same test with the parameters reversed and the difference
+    // negated
+    std::swap(x, y);
+    std::swap(xBytes, yBytes);
+    expect = -expect;
+
+    cmp = this->characterComparisonFunc(x, y, xBytes, yBytes);
+    std::memset(buf, 0, sizeof buf);
+    std::memcpy(buf[0], x, xBytes);
+    std::memcpy(buf[1], y, yBytes);
+    ASSERT_EQ(cmp, expect) << "compare '" << x << "'(" << xBytes << ") to '"
+                           << y << "'(" << yBytes << "), got " << cmp
+                           << ", should be " << expect << '\n';
+  }
+}