Page MenuHomePhabricator

[test] Split LLDB tests into filecheck, unittest and dotest.
ClosedPublic

Authored by JDevlieghere on Oct 7 2019, 3:59 PM.

Details

Summary

LLDB has three major testing strategies: unit tests, tests that exercise the SB API though dotest.py and what we currently call lit tests. The later is rather confusing as we're now using lit as the driver for all three types of tests. As most of this grew organically, the directory structure in the lldb repository doesn't really make this clear. The 'lit' tests are part of the root and among these tests there's a Unit and Suite folder for the unit and dotest-tests. This makes it impossible to run just the lit tests.

This patch changes the directory layout to match the 3 testing strategies, each with their own directory and their own configuration file. I've opted to name the directories after the testing tool, so dotest for the dotest.py tests and filecheck for the lit tests. This means there are now 3 directories under lit with 3 corresponding targets in addition to the usual check-lldb:

  • check-lldb-unittests (replaces check-lldb-unit)
  • check-lldb-dotest
  • check-lldb-filecheck

Each directory defines its own lit test suite (which was already the case). The top level directory also has a lit.cfg.py which is there for check-lldb and is mostly empty.

Current layout:

lit
├── CMakeLists.txt
├── lit-lldb-init.in
├── lit.cfg.py
├── lit.site.cfg.py.in
├── Breakpoint
├── BuildScript
├── Commands
├── Driver
├── ExecControl
├── Expr
├── Heap
├── Host
├── Minidump
├── Modules
├── ObjectFile
├── Process
├── Python
├── Quit
├── Register
├── Reproducer
├── Settings
├── Suite          <- dotest
├── SymbolFile
├── Unit           <- unittests
├── Unwind
├── Watchpoint
├── dotest
├── filecheck
├── helper
└── lldb

New layout:

lit
├── CMakeLists.txt
├── lit.cfg.py
├── dotest
├── filecheck
└── unittests

My goal is to move the unit test and dotest files into their corresponding subdirectories, remove them from the root and finally rename lit to tests. This is the first step in that direction.

Diff Detail

Event Timeline

JDevlieghere created this revision.Oct 7 2019, 3:59 PM
Herald added a project: Restricted Project. · View Herald Transcript
JDevlieghere edited the summary of this revision. (Show Details)Oct 7 2019, 4:03 PM
jingham added a subscriber: jingham.Oct 7 2019, 4:07 PM

This seems much clearer to me.

JDevlieghere edited the summary of this revision. (Show Details)Oct 7 2019, 4:13 PM
JDevlieghere edited the summary of this revision. (Show Details)Oct 7 2019, 4:25 PM
JDevlieghere edited the summary of this revision. (Show Details)Oct 7 2019, 4:25 PM

Essentially this patch just moves all the filecheck tests in a directory called filecheck and renames the Suite and Unit directories to dotest and unittests respectively.

xiaobai accepted this revision.Oct 7 2019, 4:51 PM
This revision is now accepted and ready to land.Oct 7 2019, 4:51 PM
thopre added a comment.Oct 8 2019, 1:19 AM

Shouldn't the CMake target remain check-lldb-unit to be consistant with check-llvm-unit/check-clang-unit etc.?

and finally rename lit to tests

Shouldn't that be test, to be consistent with llvm/lld/clang and others?

labath added a comment.Oct 8 2019, 6:26 AM

Shouldn't the CMake target remain check-lldb-unit to be consistant with check-llvm-unit/check-clang-unit etc.?

and finally rename lit to tests

Shouldn't that be test, to be consistent with llvm/lld/clang and others?

+1 to both of these.

More generally, there are other things here which are not consistent with llvm layouts, and I am wondering how much should try to be compatible with that. On one hand, consistency is good, but on the other, llvm does not have _three_ kinds of tests to worry about. For instance, test/Unit *is* consistent with the llvm location of unit tests, and by extension test/Suite could be considered the "right place" for non-filecheck tests too (though maybe it shouldn't be called "Suite"). Speaking of that, I am not completely sure about the name "filecheck" either -- technically this should be "shtest" as that is their main distinguishing feature, though that may end up being too pedantic.

tatyana-krasnukha accepted this revision.Oct 8 2019, 8:05 AM

I have longed for this change, thank you!

kwk added a subscriber: kwk.Oct 8 2019, 8:59 AM

We discussed this on IRC a bit and this is my updated proposal for the directory names:

  • API
  • Shell
  • Unit

The corresponding targets are lldb-check-unit (unchanged), lldb-check-shell (runs just the FileCheck / ShTests) and lldb-check-api (runs just the dotest.py tests). I've also added a README to both API and Unit explaining where the actual tests can be found.

Rename lit to test to decrease the number of renames in the git history.

labath accepted this revision.Oct 9 2019, 9:12 AM
This revision was automatically updated to reflect the committed changes.

Before this patch I was getting:

#rm -rf *
cmake ~/redhat/llvm-monorepo2/llvm/ -DCMAKE_BUILD_TYPE=Release  -DLLVM_USE_LINKER=gold -DLLVM_ENABLE_PROJECTS="lldb;clang;lld"  -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DLLVM_ENABLE_ASSERTIONS=ON
make check-lldb
...
llvm-lit: /home/jkratoch/redhat/llvm-monorepo2/llvm/utils/lit/lit/llvm/subst.py:134: fatal: Did not find count in /home/jkratoch/redhat/llvm-monorepo2-clangassert/./bin

After this patch I am getting:

[100%] Running lldb lit test suite
llvm-lit: /home/jkratoch/redhat/llvm-monorepo2/llvm/utils/lit/lit/discovery.py:133: warning: unable to find test suite for '/home/jkratoch/redhat/llvm-monorepo2-clangassert/tools/lldb/test'
llvm-lit: /home/jkratoch/redhat/llvm-monorepo2/llvm/utils/lit/lit/discovery.py:244: warning: input '/home/jkratoch/redhat/llvm-monorepo2-clangassert/tools/lldb/test' contained no tests
Testing Time: 0.00s
2 warning(s) in tests.
[100%] Built target check-lldb-lit
Scanning dependencies of target check-lldb
[100%] Built target check-lldb

A Fedora buildbot has not yet built this release: http://lab.llvm.org:8014/builders/lldb-x86_64-fedora?numbuilds=1000