[webkit-reviews] review granted: [Bug 38616] new-run-webkit-tests: clean up newline handling in printing : [Attachment 55179] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 38616: new-run-webkit-tests: clean up newline handling in printing
https://bugs.webkit.org/show_bug.cgi?id=38616

Attachment 55179: Patch
https://bugs.webkit.org/attachment.cgi?id=55179&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
WebKitTools/Scripts/webkitpy/common/array_stream.py:1
 +  #!/usr/bin/python
No need.

WebKitTools/Scripts/webkitpy/common/array_stream.py:36
 +	This is used primarily by unit test classes to mock output streams.
We should document when clients should use this instead of StringIO.  including
how this differs from StringIO.

WebKitTools/Scripts/webkitpy/common/array_stream.py:55
 +	    """Return whether the stream is empty."""
Although I think pep8 suggests these, they're often completely useless. :)

I'm not sure if is_empty() or empty() is preferred.  I know C++ webkit prefers
isEmpty().

WebKitTools/Scripts/webkitpy/common/array_stream.py:46
 +	def get(self):
StringIO calls their equivalent getvalue() which is not pep8 compliant.  But
just thought you might want to know.  If they had followed pep8, I guess I
would have suggested we change to match.

WebKitTools/Scripts/webkitpy/common/array_stream.py:63
 +	    return '<ArrayStream: ' + str(self._contents) + '>'
It's sad that every class has to implement this template.  I'm surprised object
doesn't provide some sort of hook to allow you to fill in the middle.

WebKitTools/Scripts/webkitpy/common/array_stream_unittest.py:41
 +	    self.assertTrue(a.empty())
You repeat this empty check enough times, I would have considered adding a def
_assert_empty(stream) which did those two asserts.  It would probably only net
a couple lines in the end but might make the main test method more clear. 
Unsure.

WebKitTools/Scripts/webkitpy/common/array_stream_unittest.py:55
 +	    self.assertEquals(a.get(), ["foo", "bar"])
Actually, you could easily add an assert_value() check which takes the value to
assert and could itself figure out whether to assert empty true or false.  That
would clean up several checks and make all of these places consistent.

WebKitTools/Scripts/webkitpy/common/array_stream_unittest.py:68
 +  if __name__ == '__main__':
Won't work anyway given how we import, so please remove.

WebKitTools/Scripts/webkitpy/layout_tests/layout_package/metered_stream.py:141
 +	    if len(str):
I'm not sure why len() is clearer than just if str:  but I think you and I have
had that discussion before. :)

WebKitTools/Scripts/webkitpy/layout_tests/layout_package/printing.py:486
 +	    # from the logger :(.
I think cjerdonek knows some magic.

WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:674
 +	    self._printer.print_update("All threads started ...")
Sigh.  You'll conflict with changes I have locally to this method.  We hang at
this message for several seconds for reasons I don't understand.


If you're not particularly partial to the log change, you should consider
reverting it to not conflict with bug 38561 which moves code around in that
file.

Looks OK.  Please consider updating the unit tests before landing.  I don't
need to see this again.