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

[Enrico Granata via llvm-commits]

granata.enrico added inline comments.

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

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

================
Comment at: packages/Python/lldbsuite/test/test_result.py:145
@@ +144,3 @@
+        exception = err_tuple[1]
+        return type(exception) == build_exception.BuildError
+
----------------
I would use isinstance() here

================
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)),
----------------
Could this be done by adding a __str__() or __repr__() method on BuildError instead?

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

----------------
Newline at the end :-)


http://reviews.llvm.org/D20252