This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by mzeren-vmw on Jan 14 2018, 7:13 AM.

Details

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

Diff Detail

Event Timeline

mzeren-vmw created this revision.Jan 14 2018, 7:13 AM
mzeren-vmw updated this revision to Diff 132395.Feb 1 2018, 7:22 AM
  • Reviewers: drop euhlmann, add djasper
  • Rebase
  • Ping
krasimir requested changes to this revision.Mar 27 2018, 1:38 PM

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

This revision now requires changes to proceed.Mar 27 2018, 1:38 PM

Update other verifyFormat implementations.

Update other verifyFormat implementations.

Ping?

krasimir accepted this revision.Apr 4 2018, 2:09 AM

Thank you!

This revision is now accepted and ready to land.Apr 4 2018, 2:09 AM
This revision was automatically updated to reflect the committed changes.
MyDeveloperDay added inline comments.
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?

Herald added a project: Restricted Project. · View Herald TranscriptMar 21 2019, 1:54 AM

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

mzeren-vmw added inline comments.Mar 21 2019, 8:35 PM
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.