This is an archive of the discontinued LLVM Phabricator instance.

Add more pre-run asserts for the DirCompletionAbsolute test
ClosedPublic

Authored by teemperor on Aug 22 2018, 10:57 AM.

Details

Summary

The DirCompletionAbsolute is still randomly failing on the nodes even after D50722, so this patch adds more asserts
that verify certain properties on which the actual completion implementation relies on.

The first assert checks that the directory we complete on actually exists. If the directory doesn't exist on the
next CI failure, this assert should catch it and we know that the 0 matches come from a missing base directory.

The second assert is just checking that we are below the PATH_MAX limit that the completion checks against.
This check could randomly fail if the temporary directories we generate are sometimes longer than PATH_MAX,
and the assert can tell us that this is the reason we failed (instead of the mysterious '0 matches').

(As a sidenote: We shouldn't be checking against PATH_MAX anyway in the code (as this is just wrong). Also
the disk completion API really needs a better error mechanism than returning 0 on both error or no-results.)

Diff Detail

Repository
rL LLVM

Event Timeline

teemperor created this revision.Aug 22 2018, 10:57 AM
aprantl accepted this revision.Aug 23 2018, 3:45 PM
This revision is now accepted and ready to land.Aug 23 2018, 3:45 PM
aprantl added inline comments.Aug 23 2018, 4:03 PM
unittests/Interpreter/TestCompletion.cpp
171 ↗(On Diff #161999)

This looks like it's now a line too far below?

aprantl requested changes to this revision.Aug 23 2018, 4:03 PM
This revision now requires changes to proceed.Aug 23 2018, 4:03 PM
teemperor updated this revision to Diff 162295.Aug 23 2018, 4:18 PM
  • Moved asserts to the right position.
teemperor marked an inline comment as done.Aug 23 2018, 4:19 PM
teemperor added inline comments.
unittests/Interpreter/TestCompletion.cpp
171 ↗(On Diff #161999)

Jep, nice catch! Thanks!

aprantl accepted this revision.Aug 23 2018, 4:20 PM
This revision is now accepted and ready to land.Aug 23 2018, 4:20 PM
This revision was automatically updated to reflect the committed changes.
teemperor marked an inline comment as done.

This does not build correctly on Windows (at least not in our environment). Here are the two errors:

[error]llvm\tools\lldb\unittests\interpreter\testcompletion.cpp(168,0): Error C2065: 'PATH_MAX': undeclared identifier
[error]llvm\tools\lldb\unittests\interpreter\testcompletion.cpp(168,0): Error C2512: 'testing::AssertionResult': no appropriate default constructor available

I haven't had an opportunity to investigate further yet.

@stella.stamenova Sorry, forgot about Windows here. Just fixed it in r340652.

@stella.stamenova Sorry, forgot about Windows here. Just fixed it in r340652.

Thank you!