tfiala added a comment.
I'll make some adjustments per your review, Pavel. Thanks! I'll put up
another patch set at that point.
================
Comment at: packages/Python/lldbsuite/test/dotest.py:695
@@ -694,3 +694,3 @@
# Try to match the regexp pattern, if specified.
if configuration.regexp:
----------------
labath wrote:
> This function (`visit`) is getting quite big, and due to the way it's
> structured, you were forced to duplicate some code. We could solve both
> problems with a bit of refactoring.
>
> How about this:
> - put everything inside the for loop below this line into a separate function
> (`visit_file` ?)
> - put the try-catch block around the call to `visit_file` in this function
>
> This way we will have less duplication and will reduce the nesting depth of
> this function.
>
> What do you think?
Sounds like a good change.
================
Comment at: packages/Python/lldbsuite/test/dotest.py:725
@@ +724,3 @@
+ test_filename = os.path.abspath(os.path.join(dir, name))
+ if test_filename.endswith(".pyc"):
+ test_filename = test_filename[0:-1]
----------------
labath wrote:
> Will this ever be executed? I mean, if we get a pyc file, we shouldn't even
> get here due to the check on line 689.
Ah yes, that is true. Since we assert downstream from here if it was a .pyc
file, I was being safe.
This will get refactored per previous comment, and at that point I will need it.
================
Comment at: packages/Python/lldbsuite/test_event/formatter/pickled.py:57
@@ +56,3 @@
+ # end.
+ if self.use_send:
+ # Send it as
{serialized_length_of_serialized_bytes}{serialized_bytes}
----------------
labath wrote:
> Why do we need the format to be different based on the kind of object we are
> writing to? The added magic (introspection) seems like a bad tradeoff, if all
> it does is avoid a couple of lines in `event_collector.py`
When we're using sockets, we have to be able to know the size of the full info
when reconstructing on the receiving side. This is the normal mode in which
this is used. However, that also complicates the parsing of the data for the
simple test driver.
The code later on in the test_event unit tests:
```
if os.path.exists(pickled_events_filename()):
with open(pickled_events_filename(), "rb") as events_file:
while True:
try:
# print("reading event")
event = cPickle.load(events_file)
# print("read event: {}".format(event))
if event:
events.append(event)
except EOFError:
# This is okay.
Break
```
Would get considerably more complicated to deal with the same format that is
only required for going over a network-style protocol. I prefer this tradeoff.
In the unit tests, I just send the event output to a file, and then read it
with the simple loop I included above.
However, to verify that I really prefer it, I will try writing it the other way.
================
Comment at:
packages/Python/lldbsuite/test_event/test/src/TestCatchInvalidDecorator.py:38
@@ +37,3 @@
+
+ self.assertTrue(
+ len(results) > 0,
----------------
labath wrote:
> I don't fully understand the whole event collector logic yet, but I would
> expect to see a check that the returned result is in fact an error result, no?
>
> (Also, I prefer using `assertGreater(foo, bar)`, as it provides a more
> descriptive error message.)
> .. but I would expect to see a check that the returned result is in fact an
> error result, no?
Ah yes. That is missing. (Before, we weren't getting any job_result or
test_result events).
I'll add that.
> (Also, I prefer using assertGreater(foo, bar), as it provides a more
> descriptive error message.)
Haha okay so I looked for that method when I originally worked this. The
Python 2.7 page has a link to take to the assertion methods in a table. The
table it points to doesn't have that call. However, they helpfully have two or
three more tables below that. Not sure why all those calls don't go into the
same table, particularly on a link for assertion methods from earlier in the
page.
I totally agree, I'll change that.
================
Comment at: packages/Python/lldbsuite/test_event/test/src/event_collector.py:25
@@ +24,3 @@
+def pickled_events_filename():
+ return "/tmp/lldb_test_event_pickled_event_output.dat"
+
----------------
labath wrote:
> I'm pretty sure this won't work on windows.. :)
> `tempfile.NamedTemporaryFile` looks like a portable alternative.
Oh yes. That's right. I was thinking I'd get around to trying this on Windows
myself, but that didn't happen. At that point I was going to crack out the
docs on the temp file handling.
Thanks for catching.
http://reviews.llvm.org/D20193
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
