[Lldb-commits] [PATCH] D101153: [lldb][NFC] Specify guidelines for API tests
This revision was automatically updated to reflect the committed changes. Closed by commit rG4112f5ef69a1: [lldb][NFC] Specify guidelines for API tests (authored by teemperor). Herald added a subscriber: lldb-commits. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101153/new/ 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,129 @@ 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 +
[Lldb-commits] [PATCH] D101153: [lldb][NFC] Specify guidelines for API tests
teemperor updated this revision to Diff 341832. teemperor added a comment. - Addressed Diana's comments (thanks!) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101153/new/ 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,129 @@ 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
[Lldb-commits] [PATCH] D101153: [lldb][NFC] Specify guidelines for API tests
shafik accepted this revision as: shafik. shafik added a comment. This revision is now accepted and ready to land. I don't have any other feedback, LGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101153/new/ https://reviews.llvm.org/D101153 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D101153: [lldb][NFC] Specify guidelines for API tests
rovka added a comment. This is awesome, thanks for writing this up! Comment at: lldb/docs/resources/test.rst:224 +tests the `SBProcess`, `SBThread`, and `SBFrame` classes. The same is true +for tests that exercise to breakpoints, watchpoints and sanitizers. +Languages such as Objective-C that have a dependency on a runtime I don't think you need 'to' here. Comment at: lldb/docs/resources/test.rst:256 +**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 program -> programs Comment at: lldb/docs/resources/test.rst:310 +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 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 :) Comment at: lldb/docs/resources/test.rst:313 +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. + help -> helps CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101153/new/ https://reviews.llvm.org/D101153 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D101153: [lldb][NFC] Specify guidelines for API tests
teemperor updated this revision to Diff 340897. teemperor added a comment. - Americanized the commas (Her Majesty the Queen won't be pleased, won't she?). - Just removed some justs. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101153/new/ 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,129 @@ 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 to 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 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 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 +
[Lldb-commits] [PATCH] D101153: [lldb][NFC] Specify guidelines for API tests
JDevlieghere added inline comments. Comment at: lldb/docs/resources/test.rst:229 +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). jasonmolenda wrote: > teemperor wrote: > > JDevlieghere wrote: > > > > > 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). > 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. 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!) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101153/new/ https://reviews.llvm.org/D101153 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D101153: [lldb][NFC] Specify guidelines for API tests
jasonmolenda added a comment. 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. Comment at: lldb/docs/resources/test.rst:229 +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). teemperor wrote: > JDevlieghere wrote: > > > 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). 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. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101153/new/ https://reviews.llvm.org/D101153 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D101153: [lldb][NFC] Specify guidelines for API tests
teemperor updated this revision to Diff 340520. teemperor marked 5 inline comments as done. teemperor added a comment. - Update diff with feedback. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101153/new/ 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,129 @@ 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 just 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 to 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 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
[Lldb-commits] [PATCH] D101153: [lldb][NFC] Specify guidelines for API tests
teemperor marked 7 inline comments as done.
teemperor added a comment.
Addressing feedback.
Comment at: lldb/docs/resources/test.rst:206
+
+**Don't unnecessarily launch the test executable.**
+Launching a process and running to a breakpoint can often be the most
shafik wrote:
> 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.
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.
Comment at: lldb/docs/resources/test.rst:222
+``gmodules`` need to recompile external headers when they encounter
+test-specific flags (including defines) which can be very expensive.
+
DavidSpickett wrote:
> 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.
Thanks! I clarified that specifying them anywhere is the problem (and how to
avoid it).
Comment at: lldb/docs/resources/test.rst:229
+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).
JDevlieghere wrote:
>
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).
Comment at: lldb/docs/resources/test.rst:264
+::
+self.expect("expr 1 - 1", substrs=["0"])
+
rupprecht wrote:
> shafik wrote:
> > Maybe some more examples with alternatives would be helpful here.
> 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.
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).
Comment at: lldb/docs/resources/test.rst:279-280
+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.
+
rupprecht wrote:
> nit: extra "# Good"
Thanks!
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D101153/new/
https://reviews.llvm.org/D101153
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D101153: [lldb][NFC] Specify guidelines for API tests
DavidSpickett added inline comments. Comment at: lldb/docs/resources/test.rst:222 +``gmodules`` need to recompile external headers when they encounter +test-specific flags (including defines) which can be very expensive. + 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. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101153/new/ https://reviews.llvm.org/D101153 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D101153: [lldb][NFC] Specify guidelines for API tests
rupprecht added a comment.
LGTM too, thanks for writing this up!
Comment at: lldb/docs/resources/test.rst:264
+::
+self.expect("expr 1 - 1", substrs=["0"])
+
shafik wrote:
> Maybe some more examples with alternatives would be helpful here.
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.
Comment at: lldb/docs/resources/test.rst:279-280
+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.
+
nit: extra "# Good"
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D101153/new/
https://reviews.llvm.org/D101153
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D101153: [lldb][NFC] Specify guidelines for API tests
shafik added a comment.
Thank you, this is awesome.
Comment at: lldb/docs/resources/test.rst:206
+
+**Don't unnecessarily launch the test executable.**
+Launching a process and running to a breakpoint can often be the most
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.
Comment at: lldb/docs/resources/test.rst:264
+::
+self.expect("expr 1 - 1", substrs=["0"])
+
Maybe some more examples with alternatives would be helpful here.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D101153/new/
https://reviews.llvm.org/D101153
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D101153: [lldb][NFC] Specify guidelines for API tests
JDevlieghere added a comment. 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. Comment at: lldb/docs/resources/test.rst:215 +and it makes reproducing test failures on other operating systems or +configurations harder. There is usually + Seems like the last sentence is incomplete? Comment at: lldb/docs/resources/test.rst:229 +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). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101153/new/ https://reviews.llvm.org/D101153 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D101153: [lldb][NFC] Specify guidelines for API tests
teemperor added a comment. 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). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101153/new/ https://reviews.llvm.org/D101153 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D101153: [lldb][NFC] Specify guidelines for API tests
teemperor updated this revision to Diff 339978.
teemperor added a comment.
- Improve some wording.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D101153/new/
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,89 @@
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 which
+is far from ideal:
+
+::
+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
[Lldb-commits] [PATCH] D101153: [lldb][NFC] Specify guidelines for API tests
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
