[lldb][NFC] Specify guidelines for API tests

[teemperor]

Summary

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").

[teemperor]

• Improve some wording.

[teemperor]

If anyone feels like any of the guidelines is actually controversial then let me know and I'll remove it from this review and split it out into its own patch.

(I am also aware that the text directly above has some small overlap with the guidelines as it for example also recommends the lldb utility functions for setup/checking. But given how frequently this comes up in patches I just put it in as its own guideline).

[JDevlieghere]

Thanks for taking all the lore around API tests and writing it down. I think this is going to be very useful and make our lives easier during code review.

This LGTM but I won't accept (yet) to give others a chance to see this show up in their review queue.

lldb/docs/resources/test.rst

215

Seems like the last sentence is incomplete?

229

[shafik]

Thank you, this is awesome.

lldb/docs/resources/test.rst

206

While I agree with this, it also feels unhelpful because it does not give any examples nor explain the alternatives.

I can see the problem with pointing at specific tests which may disappear or change. I can also see the problem with attempting to enumerate all the possibilities below this as well.

Maybe we need a set of example tests as well?

Most of the rest of advice stands alone pretty well though.

264

Maybe some more examples with alternatives would be helpful here.

[rupprecht]

LGTM too, thanks for writing this up!

lldb/docs/resources/test.rst

264

Also mentioning why this check is bad would help spell it out. IIUC, it's because the full output will include $0 (if it's the first expression, as noted); in which case, a stronger example is something that's clearly wrong:

(lldb) expr 5-3
(int) $0 = 2

In which case, self.expect("expr 5 - 3", substrs=["0"]) passes, but shouldn't.

279–280

nit: extra "# Good"

[DavidSpickett]

lldb/docs/resources/test.rst

222

Does this mean only use the flags you really need, or to put those flags somewhere other than the makefile? (I guess the former since they'd have to be in the makefile for your test to work anyway)

Would be good to make it clear so the reader doesn't think that there's some other place to put compiler flags, which isn't specified.

[teemperor]

Addressing feedback.

lldb/docs/resources/test.rst

206

Good point. We actually have (or had?) example tests but they always end up being forgotten about and are probably in a bad shape these days. I think by now many tests are in a good enough shape that people find a good test they can copy/extend for their own purpose, so I think we are relatively fine without explicit example tests that demonstrate this.

I updated the text with a list of functionality that I would expect to avoid creating a process in their tests (and a list of features where creating a test is unavoidable in 99% of the cases). I don't think this list will every change and I think it should give people a general idea whether they can avoid launching a process.

222

Thanks! I clarified that specifying them anywhere is the problem (and how to avoid it).

229

Done. I actually believe the comma is correct (at least the internet says it is and if that's not a reliable source than what is).

264

Thanks!

I actually tried to avoid the formatting pain of having a code example in the middle a sphinx definition block. I put an explanation and a better alternative in the text now, but I don't think I can avoid that the non-inline code example terminates the first block. The only effect of this is just that the text following the definition blocks doesn't have the same indentation as the first text block (which actually doesn't look that bad, just a mild annoyance).

279–280

Thanks!

[teemperor]

• Update diff with feedback.

[jasonmolenda]

Nice addition, these are great guidelines. I'd do a s/just// pass over the text, there's some extraneous "just"s that could go. This is a very bad habit of my own lately, I re-read emails I write, and try to remove "justs" that snuck into the text I typed before I send. I think my keyboard driver might be responsible for inserting them into sentences.

lldb/docs/resources/test.rst

229

two cents - if it were english, like "(that is, do not generate", I'd use a comma. I've never thought about which is correct with "i.e." but if I were thinking about it, I'd go with a comma.

[JDevlieghere]

lldb/docs/resources/test.rst

229

Interesting, so this is a British/American English thing, where the former generally does not use a comma while the latter does. I learned something new today :-) Anyway, well free to add the comma again as the docs are all using American English. (sorry for the churn!)

[teemperor]

• Americanized the commas (Her Majesty the Queen won't be pleased, won't she?).
• Just removed some justs.

[rovka]

This is awesome, thanks for writing this up!

lldb/docs/resources/test.rst

224

I don't think you need 'to' here.

256

program -> programs

310

You're using 'see' twice in the same sentence. Some alternatives:
"Check the documentation [...]",
"Read the documentation [...]",
"[...] learn what asserts are available",
or anything else along those lines :)

313

help -> helps

[shafik]

I don't have any other feedback, LGTM.

[teemperor]

• Addressed Diana's comments (thanks!)

lldb/docs/resources/test.rst

 the variant as a nice-to-have.
 
 In conclusion, you'll want to opt for an API test to test the API itself or
 when you need the expressivity, either for the test case itself or for the
 program being debugged. The fact that the API tests work with different
 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 and should be avoided if possible. A large part
+    of LLDB's functionality is available directly after creating an `SBTarget`
+    of the test executable.
+
+    The part of the SB API that can be tested with just a target includes
+    everything that represents information about the executable and its
+    debug information (e.g., `SBTarget`, `SBModule`, `SBSymbolContext`,
+    `SBFunction`, `SBInstruction`, `SBCompileUnit`, etc.). For test executables
+    written in languages with a type system that is mostly defined at compile
+    time (e.g., C and C++) there is also usually no process necessary to test
+    the `SBType`-related parts of the API. With those languages it's also
+    possible to test `SBValue` by running expressions with
+    `SBTarget.EvaluateExpression` or the `expect_expr` testing utility.
+
+    Functionality that always requires a running process is everything that
+    tests the `SBProcess`, `SBThread`, and `SBFrame` classes. The same is true
+    for tests that exercise breakpoints, watchpoints and sanitizers.
+    Languages such as Objective-C that have a dependency on a runtime
+    environment also always require a running process.
+
+**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.
+
+**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), try to avoid specifying custom compiler
+    flags if possible. Certain debug information formats such as ``gmodules``
+    use a cache that is shared between all API tests and that contains
+    precompiled system headers. If you add or remove a specific compiler flag
+    in your test (e.g., adding ``-DFOO`` to the ``Makefile`` or ``self.build``
+    arguments), then the test will not use the shared precompiled header cache
+    and expensively recompile all system headers from scratch. If you depend on
+    a specific compiler flag for the test, you can avoid this issue by either
+    removing all system header includes or decorating the test function with
+    ``@no_debug_info_test`` (which will avoid running all debug information
+    variants including ``gmodules``).
+
+**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 programs 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 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 will unexpectedly pass if it's ran as the first expression in a test:
+
+::
+
+    self.expect("expr 2 + 2", substrs=["0"])
+
+When running the same command in LLDB the reason for the unexpected success
+is that '0' is found in the name of the implicitly created result variable:
+
+::
+
+    (lldb) expr 2 + 2
+    (int) $0 = 4
+           ^ The '0' substring is found here.
+
+A better way to write the test above would be using LLDB's testing function
+``expect_expr`` will only pass if the expression produces a value of 0:
+
+::
+
+    self.expect_expr("2 + 2", result_value="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. Check 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 helps 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)
+
 Running The Tests
 -----------------
 
 .. note::
 
    On Windows any invocations of python should be replaced with python_d, the
    debug interpreter, when running the test suite against a debug version of
    LLDB.