This is an archive of the discontinued LLVM Phabricator instance.

FormatTest: Provide real line number in failure messages
ClosedPublic

Authored by arichardson on Sep 1 2020, 3:34 AM.

Details

Summary

Currently a test failure always reports a line number inside verifyFormat()
which is not very helpful to see which test failed. With this change we now
emit the line number where the verify function was called. When using an
IDE such as CLion, the output now includes a clickable link that points to
the call site.

Diff Detail

Event Timeline

arichardson created this revision.Sep 1 2020, 3:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 1 2020, 3:34 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
arichardson requested review of this revision.Sep 1 2020, 3:34 AM
MyDeveloperDay added inline comments.Sep 1 2020, 7:02 AM
clang/unittests/Format/FormatTest.cpp
69–72

Nit: I'm unsure of the convention for using _ at the front of functions, I'm not a major fan others might have other opinions

72

could you add an example as to what the output would look like?

I think we could pass the File and line number to the EXPECT_EQ in the << message

arichardson added inline comments.Sep 1 2020, 7:05 AM
clang/unittests/Format/FormatTest.cpp
69–72

I just picked something that's different and makes it easy to change the calls without reformatting everything. Also happy to use verifyImpl or something like that?

72

This is what it looks like now:

/Users/alex/cheri/upstream-llvm-project/clang/unittests/Format/FormatTest.cpp:81: Failure
      Expected: Expected.str()
      Which is: "void f() { MACRO(LIST(uint64_t) *a); }"
To be equal to: format(test::messUp(Code), ObjCStyle)
      Which is: "void f() { MACRO(LIST(uint64_t) * a); }"
Google Test trace:
/Users/alex/cheri/upstream-llvm-project/clang/unittests/Format/FormatTest.cpp:7859: void f() { MACRO(LIST(uint64_t) *a); }
arichardson added inline comments.Sep 1 2020, 8:02 AM
clang/unittests/Format/FormatTest.cpp
23

This is an internal class in the current gtest.h, but upstream has now included it in the public API (https://github.com/google/googletest/commit/9c82e7745c257f38d7dd7ff8a9759ea58b6a4e89)
and documents its use here https://github.com/google/googletest/blob/master/googletest/docs/advanced.md#adding-traces-to-assertions

  • fix missing argument
MyDeveloperDay accepted this revision.Sep 4 2020, 7:57 AM

This LGTM, I've certainly been bit myself in the past with not knowing which test actually failed especially when they are all foo() something.. thank you

This revision is now accepted and ready to land.Sep 4 2020, 7:57 AM

Could this be done with a custom matcher, perhaps? (might be a bit tidier than macros with FILE and LINE manually handled, etc?