[PATCH] D97403: updated character tests to gtest

[Asher Mancinelli via Phabricator via llvm-commits]

ashermancinelli added a comment.

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 - I'll revisit all of your comments and see where I can improve upon the old tests as well.



================
Comment at: flang/unittests/RuntimeGTest/CharacterTest.cpp:63-67
+TEST_P(CharacterComparisonTestsFixture, CompareCharacters) {
+  DoCharacterComparison();
+  SwapParams();
+  DoCharacterComparison();
+}
----------------
awarzynski wrote:
> [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!
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 :)


================
Comment at: flang/unittests/RuntimeGTest/RuntimeTesting.h:17
+// Defines a CHARACTER object with padding when needed
+void SetCharacter(char *, std::size_t, const char *);
+
----------------
awarzynski wrote:
> This is currently only used in RuntimeTesting.cpp. Perhaps move there?
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97403