teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.
Thanks for the patch! I'll be extra nit-picky about the test to make up for the
great meme steal of 2021.
================
Comment at: lldb/test/API/functionalities/memory/read/TestMemoryRead.py:146
+ self.build_run_stop()
+ cmd = "memory read -f d -c 1 `&argc`"
+ res = lldb.SBCommandReturnObject()
----------------
Single use var (I guess you can't reuse that as a common prefix as the address
expression has to be the last arg).
================
Comment at: lldb/test/API/functionalities/memory/read/TestMemoryRead.py:149
+ self.ci.HandleCommand(cmd, res)
+ self.assertTrue(res.Succeeded())
+
----------------
`self.assertTrue(res.Succeeded(), "memory read failed:" + res.GetError())`
================
Comment at: lldb/test/API/functionalities/memory/read/TestMemoryRead.py:154
+
+ temp_file = tempfile.NamedTemporaryFile().name
+
----------------
I believe usually we use `self.getBuildArtifact("memory-read-output")` for
that. If you open a named temp file here (and don't close it), then the second
open can fail on Windows.
================
Comment at: lldb/test/API/functionalities/memory/read/TestMemoryRead.py:166
+ format(i, expected_line, actual_line))
+ self.assertEqual(len(expected), len(lines))
+
----------------
Can we ever hit this? We just get an exception in the for loop above (and the
exception won't be very useful to diagnose the failure).
You can do this:
```
lang=python
lines = [s.strip() for s in lines]
expected = [s.strip() for s in expected]
self.assertEqual(lines, expected)
```
(assertEqual actually explains which elements are missing/additional/different
for lists)
================
Comment at: lldb/test/API/functionalities/memory/read/TestMemoryRead.py:173
+ # Make sure the file is truncated when we run the command again.
+ self.runCmd("memory read -f d -c 1 -o '{}' `&argc`".format(temp_file))
+ check_file_content([golden_output])
----------------
Could you write some garbage into the file between the two tests here?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D99890/new/
https://reviews.llvm.org/D99890
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
