[PATCH] D98303: [flang] Update output format test to use GTest

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added a comment.

This is the first time I'm reading this and the additional comments are incredibly helpful. Thank you for all the effort @ashermancinelli !

In D98303#2615397 <https://reviews.llvm.org/D98303#2615397>, @klausler wrote:

> Has this version of the tests been validated for its ability to still find bugs in a broken runtime or decimal conversion library?

I agree that we should try to preserve this capability. This should be possible with (I might be missing something):

- GTest test fixture
- call to `RegisterCrashHandler` in `setUp`

We could re-use `CatchCrash` from `testing.cpp` (it's likely that we will need to duplicate it for a short while). I think that we could test crash handling with LenTrimKind <https://github.com/llvm/llvm-project/blob/987ee6e3cc1fb672b3ed201e72a5281c2ec88c99/flang/runtime/character.cpp#L214-L236> (try running it with some invalid arguments). More specifically, we need to trigger a call to `terminator.Crash`. This could be a implemented in a separate file IMHO (better - a separate patch).

@ashermancinelli , perhaps you could add some negative tests. This would increase the test coverage and improve the overall robustness of the tests. I would hope that for tests like `HelloWorldOutputTest` this should be rather straightforward. Perhaps trickier elsewhere.



================
Comment at: flang/unittests/RuntimeGTest/NumericalFormatTest.cpp:75
+  auto status{IONAME(EndIoStatement)(cookie)};
+  ASSERT_FALSE(status) << "hello: '" << format << "' failed, status "
+                       << static_cast<int>(status);
----------------
`ASSERT_FALSE` suggests that we are expecting this to fail, but in fact this is testing for `success` (0 in this context means success, see [[ https://github.com/llvm/llvm-project/blob/987ee6e3cc1fb672b3ed201e72a5281c2ec88c99/flang/runtime/iostat.h#L25 | here ]]. I suggest `ASSERT_EQ(status, 0)` instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98303