This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Add new helpers to make tweak tests scale better. Convert most tests. NFC
ClosedPublic

Authored by sammccall on Jul 31 2019, 10:17 AM.

Details

Summary

TweakTests.cpp has some pretty good helpers added for the first few
tweaks, but they have some limitations:

  • many assertion failures point at the wrong line
  • need lots of input/output tests, setup code is duplicated across both
  • local helpers make it hard to split the file as it grows

The new helpers in TweakTests.h are based on old ones (same operations)
but try to address these issues and generally make tests more terse
while improving error messages.

This patch converts everything except ExtractVariable (which is complex
and has changes in flight, so will be converted later).
It's LOC-neutral, despite not being able to get rid of the old helpers
until ExtractVariable is done.

Diff Detail

Repository
rL LLVM

Event Timeline

sammccall created this revision.Jul 31 2019, 10:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 31 2019, 10:17 AM

Neat, thanks for the cleanup! A few suggestions from me, no major comments.

clang-tools-extra/clangd/unittests/TweakTesting.cpp
74 ↗(On Diff #212620)

Maybe print the input in case of failure?

clang-tools-extra/clangd/unittests/TweakTesting.h
9 ↗(On Diff #212620)

NIT: add an include guard

44 ↗(On Diff #212620)

I wonder whether we could use Function and get rid of 'expression' mode completely? WDYT?
One can easily turn an expression into a statement by adding a semicolon.

86 ↗(On Diff #212620)

NIT: add a level of indent

86 ↗(On Diff #212620)

NIT: fully-qualify in a macro ::clang::clangd::TweakTest::isAvailable()

90 ↗(On Diff #212620)

NIT: add a level of indent

clang-tools-extra/clangd/unittests/TweakTests.cpp
36 ↗(On Diff #212620)

NIT: s/TODO/FIXME
the latter is used more widely in clangd codebase

sammccall updated this revision to Diff 212985.Aug 2 2019, 1:21 AM
sammccall marked 9 inline comments as done.

address comments

ilya-biryukov added inline comments.Aug 2 2019, 1:39 AM
clang-tools-extra/clangd/unittests/TweakTesting.h
44 ↗(On Diff #212620)

Unsent draft comments here?

sammccall added inline comments.Aug 2 2019, 2:01 AM
clang-tools-extra/clangd/unittests/TweakTesting.cpp
74 ↗(On Diff #212620)

This is a matcher against the input, so gtest will always print it.

clang-tools-extra/clangd/unittests/TweakTesting.h
44 ↗(On Diff #212620)

We could, but I think it obfuscates intent a little bit, and doesn't really simplify anything (we're already paying for the function wrapping logic, adding a couple more strings is ~free)

86 ↗(On Diff #212620)

Indentation is clang-format's fault. Wrapped it in the do-while-0 to fix

This revision is now accepted and ready to land.Aug 2 2019, 2:08 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 2 2019, 2:13 AM