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

[Raphael Isemann via Phabricator via lldb-commits]

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