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:
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
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 al
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.
=
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 f
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 w
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 flag
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
---
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: l
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
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).
ja
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
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.
+Languag
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
_
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
==
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://revi
16 matches
Mail list logo