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

2019-03-21 Thread Mark Zeren via Phabricator via cfe-commits 
mzeren-vmw added inline comments.



Comment at: cfe/trunk/unittests/Format/FormatTest.cpp:78
 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++

MyDeveloperDay wrote:
> @ 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 "aa()" 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  = 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?
All good points. I don't think I'll be able to get to this in the near future.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D42034



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2019-03-21 Thread Manuel Klimek via Phabricator via cfe-commits 
klimek added a comment.

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


Repository:
  rL LLVM

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

https://reviews.llvm.org/D42034



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2019-03-21 Thread MyDeveloperDay via Phabricator via cfe-commits 
MyDeveloperDay added inline comments.
Herald added a project: LLVM.



Comment at: cfe/trunk/unittests/Format/FormatTest.cpp:78
 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++

@ 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 "aa()" 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  = 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?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D42034



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2018-04-04 Thread Mark Zeren via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rL329231: [clang-format] In tests, expected code should be 
format-stable (authored by mzeren-vmw, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D42034

Files:
  cfe/trunk/unittests/Format/FormatTest.cpp
  cfe/trunk/unittests/Format/FormatTestComments.cpp
  cfe/trunk/unittests/Format/FormatTestJS.cpp
  cfe/trunk/unittests/Format/FormatTestJava.cpp
  cfe/trunk/unittests/Format/FormatTestObjC.cpp
  cfe/trunk/unittests/Format/FormatTestProto.cpp
  cfe/trunk/unittests/Format/FormatTestTextProto.cpp


Index: cfe/trunk/unittests/Format/FormatTestTextProto.cpp
===
--- cfe/trunk/unittests/Format/FormatTestTextProto.cpp
+++ cfe/trunk/unittests/Format/FormatTestTextProto.cpp
@@ -36,6 +36,7 @@
   }
 
   static void verifyFormat(llvm::StringRef Code, const FormatStyle ) {
+EXPECT_EQ(Code.str(), format(Code, Style)) << "Expected code is not 
stable";
 EXPECT_EQ(Code.str(), format(test::messUp(Code), Style));
   }
 
Index: cfe/trunk/unittests/Format/FormatTestProto.cpp
===
--- cfe/trunk/unittests/Format/FormatTestProto.cpp
+++ cfe/trunk/unittests/Format/FormatTestProto.cpp
@@ -38,6 +38,7 @@
   }
 
   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)));
   }
 };
Index: cfe/trunk/unittests/Format/FormatTestComments.cpp
===
--- cfe/trunk/unittests/Format/FormatTestComments.cpp
+++ cfe/trunk/unittests/Format/FormatTestComments.cpp
@@ -70,6 +70,7 @@
 
   void verifyFormat(llvm::StringRef Code,
 const FormatStyle  = getLLVMStyle()) {
+EXPECT_EQ(Code.str(), format(Code, Style)) << "Expected code is not 
stable";
 EXPECT_EQ(Code.str(), format(test::messUp(Code), Style));
   }
 
Index: cfe/trunk/unittests/Format/FormatTestJava.cpp
===
--- cfe/trunk/unittests/Format/FormatTestJava.cpp
+++ cfe/trunk/unittests/Format/FormatTestJava.cpp
@@ -46,6 +46,7 @@
   static void verifyFormat(
   llvm::StringRef Code,
   const FormatStyle  = 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));
   }
 };
Index: cfe/trunk/unittests/Format/FormatTest.cpp
===
--- cfe/trunk/unittests/Format/FormatTest.cpp
+++ cfe/trunk/unittests/Format/FormatTest.cpp
@@ -72,6 +72,8 @@
 
   void verifyFormat(llvm::StringRef Expected, llvm::StringRef Code,
 const FormatStyle  = 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++
Index: cfe/trunk/unittests/Format/FormatTestObjC.cpp
===
--- cfe/trunk/unittests/Format/FormatTestObjC.cpp
+++ cfe/trunk/unittests/Format/FormatTestObjC.cpp
@@ -58,6 +58,7 @@
   }
 
   void verifyFormat(StringRef Code) {
+EXPECT_EQ(Code.str(), format(Code)) << "Expected code is not stable";
 EXPECT_EQ(Code.str(), format(test::messUp(Code)));
   }
 
Index: cfe/trunk/unittests/Format/FormatTestJS.cpp
===
--- cfe/trunk/unittests/Format/FormatTestJS.cpp
+++ cfe/trunk/unittests/Format/FormatTestJS.cpp
@@ -49,14 +49,18 @@
   static void verifyFormat(
   llvm::StringRef Code,
   const FormatStyle  = 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  = 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;
   }


Index: cfe/trunk/unittests/Format/FormatTestTextProto.cpp
===
--- cfe/trunk/unittests/Format/FormatTestTextProto.cpp
+++ cfe/trunk/unittests/Format/FormatTestTextProto.cpp
@@ -36,6 +36,7 @@
   }
 
   static void verifyFormat(llvm::StringRef Code, const FormatStyle ) {
+

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

2018-04-04 Thread Krasimir Georgiev via Phabricator via cfe-commits 
krasimir accepted this revision.
krasimir added a comment.
This revision is now accepted and ready to land.

Thank you!


Repository:
  rC Clang

https://reviews.llvm.org/D42034



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2018-04-03 Thread Mark Zeren via Phabricator via cfe-commits 
mzeren-vmw added a comment.

In https://reviews.llvm.org/D42034#1051965, @mzeren-vmw wrote:

> Update other verifyFormat implementations.


Ping?


Repository:
  rC Clang

https://reviews.llvm.org/D42034



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2018-03-29 Thread Mark Zeren via Phabricator via cfe-commits 
mzeren-vmw updated this revision to Diff 140276.
mzeren-vmw added a comment.

Update other verifyFormat implementations.


Repository:
  rC Clang

https://reviews.llvm.org/D42034

Files:
  unittests/Format/FormatTest.cpp
  unittests/Format/FormatTestComments.cpp
  unittests/Format/FormatTestJS.cpp
  unittests/Format/FormatTestJava.cpp
  unittests/Format/FormatTestObjC.cpp
  unittests/Format/FormatTestProto.cpp
  unittests/Format/FormatTestTextProto.cpp


Index: unittests/Format/FormatTestTextProto.cpp
===
--- unittests/Format/FormatTestTextProto.cpp
+++ unittests/Format/FormatTestTextProto.cpp
@@ -36,6 +36,7 @@
   }
 
   static void verifyFormat(llvm::StringRef Code, const FormatStyle ) {
+EXPECT_EQ(Code.str(), format(Code, Style)) << "Expected code is not 
stable";
 EXPECT_EQ(Code.str(), format(test::messUp(Code), Style));
   }
 
Index: unittests/Format/FormatTestProto.cpp
===
--- unittests/Format/FormatTestProto.cpp
+++ unittests/Format/FormatTestProto.cpp
@@ -38,6 +38,7 @@
   }
 
   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)));
   }
 };
Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -58,6 +58,7 @@
   }
 
   void verifyFormat(StringRef Code) {
+EXPECT_EQ(Code.str(), format(Code)) << "Expected code is not stable";
 EXPECT_EQ(Code.str(), format(test::messUp(Code)));
   }
 
Index: unittests/Format/FormatTestJava.cpp
===
--- unittests/Format/FormatTestJava.cpp
+++ unittests/Format/FormatTestJava.cpp
@@ -46,6 +46,7 @@
   static void verifyFormat(
   llvm::StringRef Code,
   const FormatStyle  = 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));
   }
 };
Index: unittests/Format/FormatTestJS.cpp
===
--- unittests/Format/FormatTestJS.cpp
+++ unittests/Format/FormatTestJS.cpp
@@ -49,14 +49,18 @@
   static void verifyFormat(
   llvm::StringRef Code,
   const FormatStyle  = 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  = 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;
   }
Index: unittests/Format/FormatTestComments.cpp
===
--- unittests/Format/FormatTestComments.cpp
+++ unittests/Format/FormatTestComments.cpp
@@ -70,6 +70,7 @@
 
   void verifyFormat(llvm::StringRef Code,
 const FormatStyle  = getLLVMStyle()) {
+EXPECT_EQ(Code.str(), format(Code, Style)) << "Expected code is not 
stable";
 EXPECT_EQ(Code.str(), format(test::messUp(Code), Style));
   }
 
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -72,6 +72,8 @@
 
   void verifyFormat(llvm::StringRef Expected, llvm::StringRef Code,
 const FormatStyle  = 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++


Index: unittests/Format/FormatTestTextProto.cpp
===
--- unittests/Format/FormatTestTextProto.cpp
+++ unittests/Format/FormatTestTextProto.cpp
@@ -36,6 +36,7 @@
   }
 
   static void verifyFormat(llvm::StringRef Code, const FormatStyle ) {
+EXPECT_EQ(Code.str(), format(Code, Style)) << "Expected code is not stable";
 EXPECT_EQ(Code.str(), format(test::messUp(Code), Style));
   }
 
Index: unittests/Format/FormatTestProto.cpp
===
--- unittests/Format/FormatTestProto.cpp
+++ unittests/Format/FormatTestProto.cpp
@@ -38,6 +38,7 @@
   }
 
   static void verifyFormat(llvm::StringRef Code) {
+EXPECT_EQ(Code.str(),

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

2018-03-27 Thread Krasimir Georgiev via Phabricator via cfe-commits 
krasimir requested changes to this revision.
krasimir added a comment.
This revision now requires changes to proceed.

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


Repository:
  rC Clang

https://reviews.llvm.org/D42034



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2018-02-01 Thread Mark Zeren via Phabricator via cfe-commits 
mzeren-vmw updated this revision to Diff 132395.
mzeren-vmw added a comment.

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


Repository:
  rC Clang

https://reviews.llvm.org/D42034

Files:
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -72,6 +72,7 @@
 
   void verifyFormat(llvm::StringRef Expected, llvm::StringRef Code,
 const FormatStyle  = getLLVMStyle()) {
+EXPECT_EQ(Expected.str(), format(Expected, Style));
 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++


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -72,6 +72,7 @@
 
   void verifyFormat(llvm::StringRef Expected, llvm::StringRef Code,
 const FormatStyle  = getLLVMStyle()) {
+EXPECT_EQ(Expected.str(), format(Expected, Style));
 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++
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2018-01-14 Thread Mark Zeren via Phabricator via cfe-commits 
mzeren-vmw created this revision.
mzeren-vmw added reviewers: klimek, krasimir, euhlmann.
Herald added a subscriber: cfe-commits.

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.)


Repository:
  rC Clang

https://reviews.llvm.org/D42034

Files:
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -72,6 +72,7 @@
 
   void verifyFormat(llvm::StringRef Expected, llvm::StringRef Code,
 const FormatStyle  = getLLVMStyle()) {
+EXPECT_EQ(Expected.str(), format(Expected, Style));
 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++


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -72,6 +72,7 @@
 
   void verifyFormat(llvm::StringRef Expected, llvm::StringRef Code,
 const FormatStyle  = getLLVMStyle()) {
+EXPECT_EQ(Expected.str(), format(Expected, Style));
 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++
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits