[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

[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-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] [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. +

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

[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

[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-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

[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

[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

[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

[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.

[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

[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-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: