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

Wed Mar 10 14:37:55 PST 2021

ashermancinelli added a comment.

@klausler I think I better understand your concerns now. How would you feel about this patch if I un-removed the original test, we refine this test, and ten

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

> In D98303#2617264 <https://reviews.llvm.org/D98303#2617264>, @ashermancinelli wrote:
>
>> @klausler I reproduce all of the same test cases, so the test coverage should be identical and should catch all of the same bugs. I'd like to increase coverage, but the patch as-is should keep it the same.
>
> I would like you to demonstrate that this new test does indeed properly report test failures when they occur.

In your opinion, would some death tests be sufficient? I can demonstrate some conditions under which many of these operations result in a runtime crash with the correct message, and in my opinion that would demonstrate proper failure reporting.

================
Comment at: flang/unittests/RuntimeGTest/NumericalFormatTest.cpp:75
+  auto status{IONAME(EndIoStatement)(cookie)};
+  ASSERT_FALSE(status) << "hello: '" << format << "' failed, status "
+                       << static_cast<int>(status);
----------------
awarzynski wrote:
> `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.
This is a great point, I'll address all instances of this I can find.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98303