[Lldb-commits] [PATCH] D11816: Allow dosep.py to print full output of dotest.py, even when dotest succeeds.

[Zachary Turner via lldb-commits]

zturner added inline comments.

================
Comment at: test/dosep.py:82
@@ +81,3 @@
+    with output_lock:
+        print >> sys.stderr, stdout
+        print >> sys.stderr, "[%s] FAILED" % name
----------------
chaoren wrote:
> Why did you move this here? Could you please move stderr here too? It seems weird stderr is labeled but stdout isn't.
I played with various combinations, and this way seemed the most intuitive when you actually run it with the various options and see the output.  The important thing is to make it consistent with the behavior of `report_test_pass`.  Currently, the output will look like this:

    UNSUPPORTED: LLDB (d:\src\llvmbuild\ninja_release\bin\clang.exe-i686) :: test_sb_api_directory (TestPublicAPIHeaders.SBDirCheckerCase) (This test is only for LLDB.framework built 64-bit and !lldb.test_remote)
    ----------------------------------------------------------------------
    Ran 1 test in 0.157s

    RESULT: PASSED (0 passes, 0 failures, 0 errors, 1 skipped, 0 expected failures, 0 unexpected successes)
    [120998 refs]

    [TestPublicAPIHeaders.py] PASSED
      1 out of 387 test suites processed

It looks awkward to me to label that with stdout.  stdout is what people normally expect to see as output.  So I kind of think only stderr should be labelled.  (Even better would be to use colors, but that's a followup).

The reason I moved it before the `[%s] FAILED' line is because I think this is the easiest format for making sense of it when scanning it visually.  First you want to see the output, then you want to see whether it passed or failed, then you want to see the errors.  It's jarring to me to see the errors printed out before the message that says it failed.  By the time you finish scanning through the errors to 

================
Comment at: test/dosep.py:143
@@ +142,3 @@
+    if exit_status == 0:
+        report_test_pass(name, output[1])
+    else:
----------------
chaoren wrote:
> Do you mean `output[0]`? For consistency, should we print stderr for success too? Even if it's mostly empty.
Actually I guess I should pass both values in.  For some arbitrary reason, the actual useful information from dotest is in stderr, not stdout.  And stdout is mostly useless.


http://reviews.llvm.org/D11816