This is an archive of the discontinued LLVM Phabricator instance.

[clang-format][WIP] Run more stability FormatTests

Authored by JakeMerdichAMD on May 25 2020, 11:16 AM.



I've been seeing a lot of complaints of unstable output in the bug
tracker, and noticed that a good chunk of our tests weren't checking
this. The verifyFormat functions do this, but a lot of tests don't use
these, and they miss out on the stability checks.

This change replaces all instances of EXPECT_EQ(..., format(...)) with a
verifyFormat equivalent (I'll share the regex if you like; it's
horrifying). It also narrows the criteria to rerun a test with
objective-c in verifyFormat, since some of the migrated tests were
(correctly) failing when run with ObjC vs Cpp.

WIP because:

  1. Several tests are failing because their expected output is unstable, and the underlying issues there need to be fixed.
  2. It's harder to differenciate the expected vs input text than using EXPECT_EQ. Ideas?
  3. This only does it for FormatTests.cpp. Needs to be done on the rest.

I wanted to get some feedback and general opinions on this as I debug
the underlying issues, as well as to encourage any new tests to be
written with verifyFormat instead of EXPECT_EQ. Thoughts?

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMay 25 2020, 11:16 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
JakeMerdichAMD added a project: Restricted Project.May 25 2020, 11:17 AM
MyDeveloperDay accepted this revision.May 25 2020, 12:27 PM

I think this is a noble effort, assuming all the tests pass as far as I'm concerned this LGTM.

How about committing all the ones that pass, then work on the others that don't separately.

This revision is now accepted and ready to land.May 25 2020, 12:27 PM

@HazardyKnusperkeks, @curdeius, @owenpan I feel we should try and get this committed, people tend to follow the adjacent style of the unit tests, and I sort of feel we keep having to ask people to use verifyFormat. maybe if we got rid of the unnecessary EXPECT_EQ we could reduce that, (I also like the way it messes up the code and checks its stable)

It says tests are failing. But generally this seems to be a good idea. So one should take the tests one by one. I would do it, but currently I'm trying to wrap my head around some of the internals, to get my requires clauses correctly formatted. ;)

@JakeMerdichAMD are you still interested in landing this, if not I'm happy to NFC it in over the next couple of weeks, if others agree