Page MenuHomePhabricator

[flang] Unittests for runtime terminator
ClosedPublic

Authored by ashermancinelli on Mar 15 2021, 12:48 PM.

Details

Summary

Create test fixture for runtime tests which enables verification
of failure cases. Test some runtime IO APIs for failure cases.
Support testing efforts in D98303. Expand on effort discussed
in D98601.

Diff Detail

Event Timeline

ashermancinelli requested review of this revision.Mar 15 2021, 12:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2021, 12:48 PM

Thank you for working on this!

This is a very neat validation of the crash handling API. It also demonstrates that the GTest API in no way obfuscates that crash output. In other words, this shows that we can rely on Fortran::runtime::Terminator::RegisterCrashHandler in other GTest unit tests.

I've left a few comments inline.

[nit] Could you clarify somewhere _why_ testing these particular APIs?

Otherwise, LGTM!

flang/unittests/RuntimeGTest/CrashHandlerFixture.cpp
31–32

This is not needed, is it? AFAIK, for every unit tests a new test fixture would be generated anyway.

flang/unittests/RuntimeGTest/RuntimeCrashTest.cpp
65–66

This would be a bit more idiomatic (helps, a bit, to understand what the magic number is):

ASSERT_DEATH(IONAME(OutputInteger64)(cookie, /*x=*/0xfeedface),
    "Unknown 'C' edit descriptor in FORMAT");

Similar comment in other places.

129–138

Any chance to make it a bit more visible what the difference between the 2 blocks is? Same elsewhere.

More thoroughly comment runtime crash tests.

Fix header guards to conform to LLVM style guide

Remove superfluous comments

flang/unittests/RuntimeGTest/CrashHandlerFixture.cpp
31–32

The terminator's crash handler is static, so I think we only want to register the crash handler once in a translation unit, unless I'm mistaken.

flang/unittests/RuntimeGTest/RuntimeCrashTest.cpp
65–66

IMO, /*x=*/ does not give any additional information. However I am a fan of the /*param=*/ style - do you have a suggestion for a more descriptive parameter value to use? Also, in this case, I think the name OutputInteger64 is clear enough that we don't need the parameter name comment that badly.

129–138

I left a comment at the top outlining my rationale for performing each IO operation twice - is this sufficient in your opinion?

awarzynski accepted this revision.Mar 17 2021, 12:07 PM

LGTM, thank you!

As this patch addresses some concerns raised by @klausler in the review for D98303, let's wait at least 24hr before landing this. In case there are some additional comments.

flang/unittests/RuntimeGTest/RuntimeCrashTest.cpp
65–66

I agree and I don't have any good suggestion. Let's leave it as is.

129–138

I was referring to the fact that in e.g. OverwriteBufferComplexTest you are testing 2 APIs, whereas in OverwriteBufferComplexTest you are testing only one. That wasn't immediately obvious.

This was a nit. If you can improve (e.g. through a comment), that would be great. It's not a blocker.

This revision is now accepted and ready to land.Mar 17 2021, 12:07 PM
This revision was automatically updated to reflect the committed changes.