[Lldb-commits] [PATCH] D101153: [lldb][NFC] Specify guidelines for API tests

[Raphael Isemann via Phabricator via lldb-commits]

teemperor created this revision.
teemperor added a reviewer: LLDB.
teemperor added a project: LLDB.
Herald added a subscriber: JDevlieghere.
teemperor requested review of this revision.

This patch specifies a few guidelines that our API tests should follow.

The motivations for this are twofold:

1. API tests have unexpected pitfalls that especially new contributors run into when writing tests. To prevent the frustration of letting people figure those pitfalls out by trial-and-error, let's just document them briefly in one place.

2. It prevents some arguing about what is the right way to write tests. I really like to have fast and reliable API test suite, but I also don't want to be the bogeyman that has to insist in every review that the test should be rewritten to not launch a process for no good reason. It's much easier to just point to a policy document.

I omitted some guidelines that I think could be controversial (e.g., the whole "should assert message describe failure or success").


https://reviews.llvm.org/D101153

Files:
  lldb/docs/resources/test.rst


Index: lldb/docs/resources/test.rst
===================================================================
--- lldb/docs/resources/test.rst
+++ lldb/docs/resources/test.rst
@@ -196,6 +196,88 @@
 variants mean that more general tests should be API tests, so that they can be
 run against the different variants.
 
+Guidelines for API tests
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+API tests are expected to be fast, reliable and maintainable. To achieve this
+goal, API tests should conform to the following guidelines in addition to normal
+good testing practices.
+
+**Don't unnecessarily launch the test executable.**
+    Launching a process and running to a breakpoint can often be the most
+    expensive part of a test. While it's often required to have a running
+    process to test a certain feature, sometimes its possible to test a feature
+    with a (non-running) target.
+
+**Don't unnecessarily include system headers in test sources.**
+    Including external headers slows down the compilation of the test executable
+    and it makes reproducing test failures on other operating systems or
+    configurations harder. There is usually
+
+**Avoid specifying test-specific compiler flags when including system headers.**
+    If a test requires including a system header (e.g., a test for a libc++
+    formatter includes a libc++ header), always avoid specifying custom compiler
+    flags in the test's ``Makefile``. Certain debug information formats such as
+    ``gmodules`` need to recompile external headers when they encounter
+    test-specific flags (including defines) which can be very expensive.
+
+**Test programs should be kept simple.**
+    Test executables should do the minimum amount of work to bring the process
+    into the state that is required for the test. Simulating a 'real' program
+    that actually tries to do some useful task rarely helps with catching bugs
+    and makes the test much harder to debug and maintain. The test programs
+    should always be deterministic (i.e., do not generate and check against
+    random test values).
+
+**Identifiers in tests should be simple and descriptive.**
+    Often test program need to declare functions and classes which require
+    choosing some form of identifier for them. These identifiers should always
+    either be kept simple for small tests (e.g., ``A``, ``B``, ...) or have some
+    descriptive name (e.g., ``ClassWithTailPadding``, ``inlined_func``, ...).
+    Never choose identifiers that are already used anywhere else in LLVM or
+    other programs (e.g., don't name a class  ``VirtualFileSystem``, a function
+    ``llvm_unreachable``, or a namespace ``rapidxml``) as this will just mislead
+    people ``grep``'ing the LLVM repository for those strings.
+
+**Prefer LLDB testing utilities over directly working with the SB API.**
+    The ``lldbutil`` module and the ``TestBase`` class come with a large amount
+    of utility functions that can do common test setup tasks (e.g., starting a
+    test executable and running the process to a breakpoint). Using these
+    functions not only keeps the test shorter and free of duplicated code, but
+    they also follow best test suite practices and usually give much clearer
+    error messages if something goes wrong. The test utilities also contain
+    custom asserts and checks that should be preferably used (e.g.
+    ``self.assertSuccess``).
+
+**Prefer calling the SB API over checking command output.**
+    Avoid writing your tests on top of ``self.expect(...)`` calls that check
+    the output of LLDB commands and instead try calling into the SB API. Relying
+    on LLDB commands makes changing (and improving) the output/syntax of
+    commands harder and the resulting tests are often prone to accepting
+    incorrect test results. Especially improved error messages that contain
+    more information might cause these ``self.expect`` calls to unintentionally
+    find the required ``substrs``. For example, the following ``self.expect``
+    check always passes as long as it's the first expression in a test:
+
+::
+    self.expect("expr 1 - 1", substrs=["0"])
+
+**Prefer using specific asserts over the generic assertTrue/assertFalse.**.
+    The `self.assertTrue`/`self.assertFalse` functions should always be your
+    last option as they give non-descriptive error messages. The test class has
+    several expressive asserts such as `self.assertIn` that automatically
+    generate an explanation how the received values differ from the expected
+    ones. See the documentation of Python's `unittest` module to see what
+    asserts are available. If you can't find a specific assert that fits your
+    needs and you fall back to a generic assert, make sure you put useful
+    information into the assert's `msg` argument that help explain the failure.
+
+::
+    # Bad. Will print a generic error such as 'False is not True'.
+    self.assertTrue(expected_string in list_of_results)
+    # Good. Will print expected_string and the contents of list_of_results.
+    self.assertIn(expected_string, list_of_results) # Good.
+
 Running The Tests
 -----------------
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D101153.339972.patch
Type: text/x-patch
Size: 5153 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20210423/1ee7da00/attachment-0001.bin>