FormatTest: Provide real line number in failure messages

[arichardson]

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.

[MyDeveloperDay]

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]

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]

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

[arichardson]

• fix missing argument

[MyDeveloperDay]

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

[dblaikie]

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

clang/unittests/Format/FormatTest.cpp

 #include "llvm/Support/Debug.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "gtest/gtest.h"
 
 #define DEBUG_TYPE "format-test"
 
 using clang::tooling::ReplacementTest;
 using clang::tooling::toReplacements;
+using testing::internal::ScopedTrace;
 
 namespace clang {
 namespace format {
 namespace {
 
 FormatStyle getGoogleStyle() { return getGoogleStyle(FormatStyle::LK_Cpp); }
 
 class FormatTest : public ::testing::Test {
protected:
   FormatStyle getLLVMStyleWithColumns(unsigned ColumnLimit) {
     return getStyleWithColumns(getLLVMStyle(), ColumnLimit);
   }
 
   FormatStyle getGoogleStyleWithColumns(unsigned ColumnLimit) {
     return getStyleWithColumns(getGoogleStyle(), ColumnLimit);
   }
 
-  void verifyFormat(llvm::StringRef Expected, llvm::StringRef Code,
+  void _verifyFormat(const char *File, int Line, llvm::StringRef Expected,
+                     llvm::StringRef Code,
                      const FormatStyle &Style = getLLVMStyle()) {
+    ScopedTrace t(File, Line, ::testing::Message() << Code.str());
     EXPECT_EQ(Expected.str(), format(Expected, Style))
         << "Expected code is not stable";
     EXPECT_EQ(Expected.str(), format(Code, Style));
     if (Style.Language == FormatStyle::LK_Cpp) {
       // Objective-C++ is a superset of C++, so everything checked for C++
       // needs to be checked for Objective-C++ as well.
       FormatStyle ObjCStyle = Style;
       ObjCStyle.Language = FormatStyle::LK_ObjC;
       EXPECT_EQ(Expected.str(), format(test::messUp(Code), ObjCStyle));
     }
   }
 
-  void verifyFormat(llvm::StringRef Code,
+  void _verifyFormat(const char *File, int Line, llvm::StringRef Code,
                      const FormatStyle &Style = getLLVMStyle()) {
-    verifyFormat(Code, test::messUp(Code), Style);
+    _verifyFormat(File, Line, Code, test::messUp(Code), Style);
   }
 
-  void verifyIncompleteFormat(llvm::StringRef Code,
+  void _verifyIncompleteFormat(const char *File, int Line, llvm::StringRef Code,
                                const FormatStyle &Style = getLLVMStyle()) {
+    ScopedTrace t(File, Line, ::testing::Message() << Code.str());
     EXPECT_EQ(Code.str(),
               format(test::messUp(Code), Style, SC_ExpectIncomplete));
   }
 
-  void verifyGoogleFormat(llvm::StringRef Code) {
-    verifyFormat(Code, getGoogleStyle());
-  }
-
-  void verifyIndependentOfContext(llvm::StringRef text) {
-    verifyFormat(text);
-    verifyFormat(llvm::Twine("void f() { " + text + " }").str());
+  void _verifyIndependentOfContext(const char *File, int Line,
+                                   llvm::StringRef Text,
+                                   const FormatStyle &Style = getLLVMStyle()) {
+    _verifyFormat(File, Line, Text, Style);
+    _verifyFormat(File, Line, llvm::Twine("void f() { " + Text + " }").str(),
+                  Style);
   }
 
   /// \brief Verify that clang-format does not crash on the given input.
   void verifyNoCrash(llvm::StringRef Code,
                      const FormatStyle &Style = getLLVMStyle()) {
     format(Code, Style, SC_DoNotCheck);
   }
 
   int ReplacementCount;
 };
 
+#define verifyIndependentOfContext(...)                                        \
+  _verifyIndependentOfContext(__FILE__, __LINE__, __VA_ARGS__)
+#define verifyIncompleteFormat(...)                                            \
+  _verifyIncompleteFormat(__FILE__, __LINE__, __VA_ARGS__)
+#define verifyFormat(...) _verifyFormat(__FILE__, __LINE__, __VA_ARGS__)
+#define verifyGoogleFormat(Code) verifyFormat(Code, getGoogleStyle())
+
 TEST_F(FormatTest, MessUp) {
   EXPECT_EQ("1 2 3", test::messUp("1 2 3"));
   EXPECT_EQ("1 2 3\n", test::messUp("1\n2\n3\n"));
   EXPECT_EQ("a\n//b\nc", test::messUp("a\n//b\nc"));
   EXPECT_EQ("a\n#b\nc", test::messUp("a\n#b\nc"));
   EXPECT_EQ("a\n#b c d\ne", test::messUp("a\n#b\\\nc\\\nd\ne"));
 }