[clang-format] In tests, expected code should be format-stable

[mzeren-vmw]

Summary

Extend the verifyFormat helper function to check that the expected text
is "stable". This provides some protection against bugs where formatting
results are ocilating between two forms, or continually change in some
other way.

Testing Done:

• Ran unit tests.

• Reproduced a known instability in preprocessor indentation which was caught by this new check (to be resolved in a later change.)

[mzeren-vmw]

• Reviewers: drop euhlmann, add djasper
• Rebase
• Ping

[krasimir]

Thank you!
If this works, we should apply it to all files in unittest/Format.

[mzeren-vmw]

Update other verifyFormat implementations.

[mzeren-vmw]

In D42034#1051965, @mzeren-vmw wrote:

Update other verifyFormat implementations.

Ping?

[krasimir]

Thank you!

[MyDeveloperDay]

cfe/trunk/unittests/Format/FormatTest.cpp

78 ↗

(On Diff #141054)

@ mzeren-vmw just a minor comment related to a change you made last year.

When FormatTest fails, all we are told is that it fails on line 75,or 77 (currently in trunk its 48,49) for every failure, with so many test cases written as "foo()" or "aaaaaa()" it can often be hard to pinpoint the exact test failure.

If verifyFormat was given an additional default argument of int line

Then verifyFormat could be used via a macro

#define VERIFYFORMAT(expected,code,style)
             verifyFormat(expected,code,style,__LINE__);

The line numbers could then be passed as an additional failure message to gtest by passing the LINE of the test location down.

void verifyFormat(llvm::StringRef Expected, llvm::StringRef Code,
                    const FormatStyle &Style = getLLVMStyle(),int line=__LINE__) {
    EXPECT_EQ(Expected.str(), format(Expected, Style))
        << "Expected code is not stable at " << line;
    EXPECT_EQ(Expected.str(), format(Code, Style) << " at " << line;
}

When the test fails we'd know the exact line of the test case that failed.

Also because of this, we get 2 failures for every failure, the second will almost always fail as well if the first case does (from what I can tell), is there anyway we can query the first failed result and not bother doing the second if the first one failed?

[klimek]

Great idea! LG from my side after addressing MyDeveloperDay's comments.

[mzeren-vmw]

cfe/trunk/unittests/Format/FormatTest.cpp

78 ↗

(On Diff #141054)

All good points. I don't think I'll be able to get to this in the near future.

unittests/Format/FormatTest.cpp

protected:
   }
 
   FormatStyle getGoogleStyleWithColumns(unsigned ColumnLimit) {
     return getStyleWithColumns(getGoogleStyle(), ColumnLimit);
   }
 
   void verifyFormat(llvm::StringRef Expected, llvm::StringRef Code,
                     const FormatStyle &Style = getLLVMStyle()) {
+    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));
     }

unittests/Format/FormatTestComments.cpp

protected:
   FormatStyle getTextProtoStyleWithColumns(unsigned ColumnLimit) {
     FormatStyle Style = getGoogleStyle(FormatStyle::FormatStyle::LK_TextProto);
     Style.ColumnLimit = ColumnLimit;
     return Style;
   }
 
   void verifyFormat(llvm::StringRef Code,
                     const FormatStyle &Style = getLLVMStyle()) {
+    EXPECT_EQ(Code.str(), format(Code, Style)) << "Expected code is not stable";
     EXPECT_EQ(Code.str(), format(test::messUp(Code), Style));
   }
 
   void verifyGoogleFormat(llvm::StringRef Code) {
     verifyFormat(Code, getGoogleStyle());
   }
 
   /// \brief Verify that clang-format does not crash on the given input.

unittests/Format/FormatTestJS.cpp

static FormatStyle getGoogleJSStyleWithColumns(unsigned ColumnLimit) {
     FormatStyle Style = getGoogleStyle(FormatStyle::LK_JavaScript);
     Style.ColumnLimit = ColumnLimit;
     return Style;
   }
 
   static void verifyFormat(
       llvm::StringRef Code,
       const FormatStyle &Style = getGoogleStyle(FormatStyle::LK_JavaScript)) {
+    EXPECT_EQ(Code.str(), format(Code, Style))
+        << "Expected code is not stable";
     std::string Result = format(test::messUp(Code), Style);
     EXPECT_EQ(Code.str(), Result) << "Formatted:\n" << Result;
   }
 
   static void verifyFormat(
       llvm::StringRef Expected,
       llvm::StringRef Code,
       const FormatStyle &Style = getGoogleStyle(FormatStyle::LK_JavaScript)) {
+    EXPECT_EQ(Expected.str(), format(Expected, Style))
+        << "Expected code is not stable";
     std::string Result = format(Code, Style);
     EXPECT_EQ(Expected.str(), Result) << "Formatted:\n" << Result;
   }
 };
 
 TEST_F(FormatTestJS, BlockComments) {
   verifyFormat("/* aaaaaaaaaaaaa */ aaaaaaaaaaaaaaaaaaaaaaaaaaa(\n"
                "    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa);");

unittests/Format/FormatTestJava.cpp

static FormatStyle getStyleWithColumns(unsigned ColumnLimit) {
     FormatStyle Style = getGoogleStyle(FormatStyle::LK_Java);
     Style.ColumnLimit = ColumnLimit;
     return Style;
   }
 
   static void verifyFormat(
       llvm::StringRef Code,
       const FormatStyle &Style = getGoogleStyle(FormatStyle::LK_Java)) {
+    EXPECT_EQ(Code.str(), format(Code, Style)) << "Expected code is not stable";
     EXPECT_EQ(Code.str(), format(test::messUp(Code), Style));
   }
 };
 
 TEST_F(FormatTestJava, NoAlternativeOperatorNames) {
   verifyFormat("someObject.and();");
 }
 

unittests/Format/FormatTestObjC.cpp

std::string format(llvm::StringRef Code,
     }
     auto Result = applyAllReplacements(Code, Replaces);
     EXPECT_TRUE(static_cast<bool>(Result));
     DEBUG(llvm::errs() << "\n" << *Result << "\n\n");
     return *Result;
   }
 
   void verifyFormat(StringRef Code) {
+    EXPECT_EQ(Code.str(), format(Code)) << "Expected code is not stable";
     EXPECT_EQ(Code.str(), format(test::messUp(Code)));
   }
 
   void verifyIncompleteFormat(StringRef Code) {
     EXPECT_EQ(Code.str(), format(test::messUp(Code), SC_ExpectIncomplete));
   }
 
   FormatStyle Style;

unittests/Format/FormatTestProto.cpp

protected:
 
   static std::string format(llvm::StringRef Code) {
     FormatStyle Style = getGoogleStyle(FormatStyle::LK_Proto);
     Style.ColumnLimit = 60; // To make writing tests easier.
     return format(Code, 0, Code.size(), Style);
   }
 
   static void verifyFormat(llvm::StringRef Code) {
+    EXPECT_EQ(Code.str(), format(Code)) << "Expected code is not stable";
     EXPECT_EQ(Code.str(), format(test::messUp(Code)));
   }
 };
 
 TEST_F(FormatTestProto, FormatsMessages) {
   verifyFormat("message SomeMessage {\n"
                "  required int32 field1 = 1;\n"
                "}");

unittests/Format/FormatTestTextProto.cpp

static std::string format(llvm::StringRef Code, unsigned Offset,
     return *Result;
   }
 
   static std::string format(llvm::StringRef Code, const FormatStyle &Style) {
     return format(Code, 0, Code.size(), Style);
   }
 
   static void verifyFormat(llvm::StringRef Code, const FormatStyle &Style) {
+    EXPECT_EQ(Code.str(), format(Code, Style)) << "Expected code is not stable";
     EXPECT_EQ(Code.str(), format(test::messUp(Code), Style));
   }
 
   static void verifyFormat(llvm::StringRef Code) {
     FormatStyle Style = getGoogleStyle(FormatStyle::LK_TextProto);
     Style.ColumnLimit = 60; // To make writing tests easier.
     verifyFormat(Code, Style);
   }