[PATCH] D20252: surface build error content through test event system

[Todd Fiala via llvm-commits]

tfiala added a comment.

Great feedback.  I'll address them in a follow-up patch.


================
Comment at: packages/Python/lldbsuite/test/lldbtest.py:422
@@ -421,2 +421,3 @@
                 cmd = shellCommand
-            raise CalledProcessError(retcode, cmd)
+            cpe = CalledProcessError(retcode, cmd)
+            # Ensure caller can access the stdout/stderr
----------------
granata.enrico wrote:
> I don't have a strong opinion either way, this is just me offering an alternative
> 
> Does it make sense to have custom members on the CalledProcessError object? It is not a big deal, unless some of our member names conflict with ones that Python may want to use - but in general, if we think we may want to expand the set, is there any advantage to custom member variables vs. attaching a custom dictionary:
> 
> cpe.LLDB_test_build_error_data = {'output' : ... }
> 
> Up to you really. I have seen and used both styles at different times.
Sure, dictionary sounds fine.

Effective Python (IIRC) recommended against making member accessors on the basis of there is no real protection against direct access to the members.  But the dictionary seems reasonable.

================
Comment at: packages/Python/lldbsuite/test/plugins/builder_base.py:109
@@ +108,3 @@
+        raise build_exception.BuildError(
+            "error when building test subject",
+            command,
----------------
granata.enrico wrote:
> Could we make BuildError's __init__ take a CalledProcessError directly, and extract data out of it as it sees fit?
Yes, I like that idea.

================
Comment at: packages/Python/lldbsuite/test/test_result.py:145
@@ +144,3 @@
+        exception = err_tuple[1]
+        return type(exception) == build_exception.BuildError
+
----------------
granata.enrico wrote:
> I would use isinstance() here
Okay, I'll have a look at that.  I know None behaves properly with type(), not sure if it does with isinstance().

================
Comment at: packages/Python/lldbsuite/test/test_result.py:160
@@ +159,3 @@
+        build_error = err[1]
+        error_description = "Error when building test subject.\n\nTest Directory:\n{}\n\nBuild Command:\n{}\n\nBuild Output:\n{}".format(
+            os.path.dirname(self._getTestPath(test)),
----------------
granata.enrico wrote:
> Could this be done by adding a __str__() or __repr__() method on BuildError instead?
Sure.  Removes duplication in another location (xUnit formatter).  Good call.

================
Comment at: packages/Python/lldbsuite/test_event/build_exception.py:6
@@ +5,1 @@
+        self.build_error = build_output
\ No newline at end of file

----------------
granata.enrico wrote:
> Newline at the end :-)
Ah yes.  I unintentionally forgot to add this initially (git add was forgotten), and I blew it away when preparing to put this up for review.  This one I rewrote too fast.


http://reviews.llvm.org/D20252