[Lldb-commits] [PATCH] D20193: test infra: catch bad decorators and import-time errors

[Pavel Labath via lldb-commits]

labath added a comment.

I'm glad to see this getting fixed. I have a couple comments though. :)


================
Comment at: packages/Python/lldbsuite/test/dotest.py:695
@@ -694,3 +694,3 @@
 
             # Try to match the regexp pattern, if specified.
             if configuration.regexp:
----------------
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?

================
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]
----------------
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.

================
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}
----------------
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`

================
Comment at: packages/Python/lldbsuite/test_event/test/src/TestCatchInvalidDecorator.py:38
@@ +37,3 @@
+
+        self.assertTrue(
+            len(results) > 0,
----------------
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.)

================
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"
+
----------------
I'm pretty sure this won't work on windows.. :)
`tempfile.NamedTemporaryFile` looks like a portable alternative.


http://reviews.llvm.org/D20193