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

[Todd Fiala via lldb-commits]

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